-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update to dart sass #43
Conversation
Underlying the initial error, a switch to dart-sass is in order, because dart-sass is deprecated. The dependency rfs is not updated yet and will need to be updated in future: twbs/rfs#363 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Just unsure if this can break older projects using und-css. If you need this as a hot fix feel free to merge.
If not I'd suggest to also get rid of rfs (replacing it with something like utopia.fyi) before bumping to 1.0.0
@@ -33,7 +33,7 @@ | |||
"cross-env": "^7.0.2", | |||
"css-loader": "^4.2.0", | |||
"gh-pages": "^3.1.0", | |||
"node-sass": "^4.14.1", | |||
"sass": "^1.54.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Unsure though if this will break older projects using und-css that are still in node-sass.
I would consider adding sass as a peer dependency as well and doing a major version of und-css (1.0.0) so yarn won't install this version into an older project by accident.
@@ -1,6 +1,6 @@ | |||
$generate-doc-comments: true; | |||
|
|||
@import "./src/styles"; | |||
@import "//src/styles"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with SASS' latest changes. Is this the way to do it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently. This is the solution that worked after a quick research.
Just make sure to also update the Changelog before merging |
Doing a mayor version sounds like a good idea. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good to me! 👍🏻
Thanks so much for this. I'll merge and use this as the basis for the other things needed towards a 1.0 release.
With an up do date sass loader this deprecation warning is fired:
https://sass-lang.com/documentation/breaking-changes/slash-div