-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
refactor: added &{} rules for styles below nested rules to follow [email protected]
ruleset.
#17141
refactor: added &{} rules for styles below nested rules to follow [email protected]
ruleset.
#17141
Conversation
✅ Deploy Preview for v11-carbon-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 sure what the best option is relating to the stylelint rule failures with scss/selector-no-redundant-nesting-selector
, and no-duplicate-selectors
. Some related discussion in these issues:
- Invalid rules for nesting & selectors for sass 1.77.7 stylelint-scss/stylelint-scss#1020
- Fix
no-duplicate-selectors
false positives for single nesting selector stylelint/stylelint#7893
I wonder if as part of this we should validate if these styles need to come after the mixin to override values coming from the mixin. That way we'd only have the & { }
ampersand nesting where we absolutely need it. The downside with doing that is it'll be remarkably slower than applying a blanket approach. I suppose we could infer that any styles that come after a mixin should use ampersand nesting. If there's no mixin involved we could avoid ampersand nesting. What do you think after testing the waters with the accordion styles?
The other failures look formatting-related which prettier should handle for you. I have the prettier vscode extension set to format on save so I don't have to run the command manually.
Also to recap from our convo earlier today - we don't need to update any sass version ranges in peer deps. Only the hard deps and devDeps need updated.
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.
Recapping our pairing session today, I left some comments about why the snapshot was changing.
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.
This diff looks okay to me, it appears it's just changing the output of calc from multi-line to single line, and removes uneeded parens - e.g.
- "value": "calc(1.5rem +
- 0.25 *
- ((100vw - 20rem) / 46)
- )",
+ "value": "calc(1.5rem + 0.25 * (100vw - 20rem) / 46)",
So this removes 3 lines, which over the course of the entire file changes what line each selector/delcaration/property was on.
"sass@npm:^1.51.0": | ||
version: 1.62.1 |
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.
Something between these version is why the type test snapshot is changing, though I couldn't pin down exactly what in the changelog https://github.com/sass/dart-sass/blob/main/CHANGELOG.md#1621
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.
LGTM! 🚀
dbe5402
Part of #16962
added the css rules that are written after the
@include
mixins in the scss, inside &{}, which is the recommended way to write styles after breaking change introduced by sass in the version 1.77.7Changelog
New
Changed
Testing / Reviewing