-
Notifications
You must be signed in to change notification settings - Fork 52
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
Fix sass deprecations #562
base: melsumner/failing-scenarios
Are you sure you want to change the base?
Fix sass deprecations #562
Conversation
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 don't see anything strange, or duplicated code in the generated CSS, so for me it's an OK change (based on my very limited knowledge of the codebase).
tests/dummy/app/styles/app.scss
Outdated
@@ -1,10 +1,10 @@ | |||
@import 'variables/variables'; | |||
@use 'variables/_variables' as *; |
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.
[question] I don't think the underscore is needed anymore (to have Sass ignore certain files). Is it some form of convention used locally? (I see the other files are "imported/used" without the underscore.
@use 'object-patterns/object-patterns'; | ||
@use 'pages/pages'; | ||
@use 'typography/typography'; | ||
@use 'utilities/utilities'; | ||
|
||
html { |
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.
[suggestion] not related to this PR, but I would suggest to move the global declarations in a dedicated file (eg. global.scss
and import (@use
) it as first item after the variables, before the other files (components, patterns, etc).
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.
Thank you for the suggestion! I've captured it in a separate issue: #563
Since sass no longer supports
@import
, needed to switch it up to use@use
instead, which also meant adding a few additional use declarations in files that were pulling in the variables.Going to try to find someone who regularly uses sass to review since I haven't used sass in this way in a long time and I'm not entirely positive it's the correct change.