Skip to content
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: fix mixed declarations warnings on sass-embedded 1.77.7+ #15507

Closed

Conversation

chadlwilson
Copy link

@chadlwilson chadlwilson commented Aug 3, 2024

Description

Where possible, moves mixins or declarations around in definitions or re-orders declarations where the order doesn't matter to avoid sass-embedded warnings with 1.77.7+ as documented at https://sass-lang.com/documentation/breaking-changes/mixed-decls/

There is also discussion about this at sass/dart-sass#2276 where a dart-sass developer refers to recommended approach being to re-order declarations where possible, and use & {} within mixins for behaviour that will be forward and backward compatible.

Accordingly also disables the sass-lint rule requiring mixins at the start. Without deterministically ordered mixins it's not possible to cleanly avoid interleaved declarations and nested blocks without using & {} everywhere. However, if maintainers would rather, this could prefer the ugly & {} opt-ins that will maintain the deterministic ordering, but in some cases lead to less efficient CSS as there isn't the implicit "hoisting" of declarations going on.

Methodology was to run yarn run test and correct all the sass-embedded warnings that appeared there.

Types of changes

  • Maintenance (refactor, code cleaning, development tools...)

Checklist

  • I have read and follow the CONTRIBUTING.md document.
  • The pull request title and template are correctly filled.
  • The pull request targets the right branch (develop or develop-v...).
  • My commits are correctly titled and contain all relevant information.
  • I have updated the documentation accordingly to my changes (if relevant).
  • I have added tests to cover my changes (if relevant).

Where possible, moves mixins that have nested declarations later in definitions or
re-orders declarations where the order doesn't matter.

Accordingly, disables the sass-lint rule. Without deterministically ordered mixins
it's not possible to cleanly avoid interleaved declarations and nested blocks without
using `& {}` everywhere.
Copy link

sonarcloud bot commented Aug 3, 2024

@chadlwilson
Copy link
Author

chadlwilson commented Aug 6, 2024

@joeworkman Hi there - do you not review/accept PRs on this repo? it seems you have made similar changes on develop subsequent to me submitting this PR which probably makes this obsolete now.

Spent quite a bit of time on this as well as raising the issue at #15506 so would be good to know if not worth bothering in future.

@joeworkman
Copy link
Member

Oh man, we were working on this at the same exact time! I am sorry. I was chatting about it a little on the Foundation discord channel. I will review your changes to see if there is anything that could have been different than what I did.

I am working with the Sass team with some other enhancements. They would like to move away from @import to @use. However, it's turning out to be a much larger change than I thought it might be. If you have any experience with this, I would love it if you could chime in on the discord chat.

@chadlwilson
Copy link
Author

Ahh ok, this was a few days ago now.

You're certainly more of a sass expert than me so I'd defer to your judgment. In some cases I was looking for places where the declarations were overriding something in an include where reordering would change semantics (eg. some buttons). Probably similar to your approach I imagine although I did have to disable a lint rule as a result in some places.

@chadlwilson chadlwilson closed this Aug 6, 2024
@chadlwilson chadlwilson deleted the sass-177-warnings branch August 15, 2024 15:48
@chadlwilson
Copy link
Author

@joeworkman any chance of cutting a patch release with these fixes? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Address sass 1.77.7+ warnings on mixed declarations with nested rules
2 participants