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

Fix Sass compilation when $color-mode-type is set to media-query #37687

Merged
merged 3 commits into from Dec 21, 2022

Conversation

julien-deramond
Copy link
Member

@julien-deramond julien-deramond commented Dec 20, 2022

Description

As a summary, this PR fixes Sass compilation when $color-mode-type is set to media-query.

First error

So the first error mentioned in the issue was the following:

Compilation Error
Error: Declarations may only be used within style rules.
    ╷
135 │     --#{$prefix}body-color: #{$body-color-dark};
    │     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ╵
  c:\wamp64\www\f3-5\node_modules\bootstrap\scss\_root.scss 135:5             @content
  c:\wamp64\www\f3-5\node_modules\bootstrap\scss\mixins\_color-mode.scss 5:7  color-mode()
  c:\wamp64\www\f3-5\node_modules\bootstrap\scss\_root.scss 133:3             @import
  c:\wamp64\www\f3-5\src\scss\style.scss 25:9                                 root stylesheet
--------------------

The main reason is that we had in _root.scss:

@if $enable-dark-mode {
  @include color-mode(dark, true) {
     --#{$prefix}whatever: #{$whatever};
  }
}

So when we set $color-mode-type to media-query we end up with a generated CSS for this part without any scope!

I haven't found a better way for now to fix it so I've modified our color-mode mixin to:

  • Remove the second parameter since $color-mode-type is available globally even within the mixin
  • Add a new second parameter to handle this specific use case where we want to add the :root scope

Second and third errors

After having fixed this one, another type of error popped out (I don't have it, I forgot to register it) but that said that it's not possible to use @extend within @media.
So our @extend in close button and carousels didn't work.
Instead of using @extend I've found a way (maybe not ideal) to replace it with a mixin.

I kept the test() and test2() just for you to spot easily how it works, and also because I haven't found the proper wording :)

Type of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • All new and existing tests passed

Live previews

⚠️ Big warning regarding non-regression testing. It seems to still work for $color-media-type set to data and I tried within the doc to play with the media-query but our doc is not really very usable when we enable the media-query as is; so it was complicated to ensure that it works correctly. I might need someone to double check if it is OK 🙏

Related issues

Closes #37673

@mdo
Copy link
Member

mdo commented Dec 20, 2022

Pushed a change to rename the mixins and fix the docs when moving to media-query. Should be good to go now!

@julien-deramond julien-deramond marked this pull request as ready for review December 21, 2022 06:48
@julien-deramond julien-deramond requested a review from a team as a code owner December 21, 2022 06:48
@julien-deramond
Copy link
Member Author

Sounds good, thanks for the changes to finalize it!

@XhmikosR XhmikosR merged commit 41f62c5 into main Dec 21, 2022
@XhmikosR XhmikosR deleted the main-jd-fix-color-mode-media-query branch December 21, 2022 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Setting $color-mode-type to media-query (rather than data) causes a Sass compilation erro
3 participants