-
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
Clarify unary for Sass #17595
Clarify unary for Sass #17595
Conversation
All contributors have signed the DCO. |
I have read the DCO document and I hereby sign the DCO. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17595 +/- ##
=======================================
Coverage 76.98% 76.98%
=======================================
Files 409 409
Lines 14001 14001
Branches 4353 4353
=======================================
Hits 10779 10779
Misses 3049 3049
Partials 173 173 ☔ View full report in Codecov by Sentry. |
It seems my initial fix _did_ break the styling. This commit tries the other interpretation to see if it passes the snapshot tests.
Looking at the failed snapshot, it seems the change did break the styling. I've switched to the other interpretation of the syntax. EDIT: Actually I retract this. I assumed the Percy diff was my branch against main, but it's actually my branch against main 5 days ago. It seems at least some of the diff is due to the font changing, which I doubt was caused my style change. I'm reverting 8e1ac73. |
This reverts commit 8e1ac73.
Sorry for the flaky percy report, it definitely wasn't related to your changes. All should be good here to merge once status checks pass |
@tay1orjones no worries, thanks both for reviewing! |
58290ff
* Fix: Clarify SCSS * Try other interpretation It seems my initial fix _did_ break the styling. This commit tries the other interpretation to see if it passes the snapshot tests. * Revert "Try other interpretation" This reverts commit 8e1ac73. --------- Co-authored-by: Taylor Jones <[email protected]>
Closes #17593
https://sass-lang.com/documentation/breaking-changes/strict-unary/
This line causes a deprecation warning in newer versions of Sass. This PR makes a small edit to solve the warning. This shouldn't change the behaviour of the style as Sass interprets the syntax as a subtraction (EDIT: This did not have no effect as I thought, see my comment). This is also most likely what the original author intended, otherwise they could have omitted the second
-$spacing-05
.Changelog
Changed
Testing / Reviewing
Probably easiest way to verify this is to go to the CodeSandbox repro and add the space to
node_modules/@carbon/styles/scss/components/modal/_modal.scss (L128)
.