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 use of declarations after nested rules (deprecated in Sass 1.77.7) #40623

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

julien-deramond
Copy link
Member

@julien-deramond julien-deramond commented Jul 10, 2024

(Edit: After the creation of this PR, [email protected] has been released, but it doesn't change anything to this PR that still can be merged as is.)


Description

When using [email protected], we have some warnings when running npm run css. More info at Sass: Breaking Change: Mixed Declarations.

If we simply move some calls to mixins in our code, the warnings are not displayed anymore. The related files are:

  • scss/_modal.scss
  • scss/_reboot.scss
  • scss/_type.scss

A simple way to measure the possible impact is to compare the built bootstrap.css from the main branch, and this branch:

--- dist/css/bootstrap.css	2024-07-10 18:33:12
+++ /tmp/bootstrap.main.css	2024-07-10 18:32:55
@@ -515,12 +515,12 @@
 legend {
   float: left;
   width: 100%;
   padding: 0;
   margin-bottom: 0.5rem;
-  line-height: inherit;
   font-size: calc(1.275rem + 0.3vw);
+  line-height: inherit;
 }
 @media (min-width: 1200px) {
   legend {
     font-size: 1.5rem;
   }
\ No newline at end of file
@@ -599,68 +599,68 @@
   font-size: 1.25rem;
   font-weight: 300;
 }
 
 .display-1 {
+  font-size: calc(1.625rem + 4.5vw);
   font-weight: 300;
   line-height: 1.2;
-  font-size: calc(1.625rem + 4.5vw);
 }
 @media (min-width: 1200px) {
   .display-1 {
     font-size: 5rem;
   }
 }
 
 .display-2 {
+  font-size: calc(1.575rem + 3.9vw);
   font-weight: 300;
   line-height: 1.2;
-  font-size: calc(1.575rem + 3.9vw);
 }
 @media (min-width: 1200px) {
   .display-2 {
     font-size: 4.5rem;
   }
 }
 
 .display-3 {
+  font-size: calc(1.525rem + 3.3vw);
   font-weight: 300;
   line-height: 1.2;
-  font-size: calc(1.525rem + 3.3vw);
 }
 @media (min-width: 1200px) {
   .display-3 {
     font-size: 4rem;
   }
 }
 
 .display-4 {
+  font-size: calc(1.475rem + 2.7vw);
   font-weight: 300;
   line-height: 1.2;
-  font-size: calc(1.475rem + 2.7vw);
 }
 @media (min-width: 1200px) {
   .display-4 {
     font-size: 3.5rem;
   }
 }
 
 .display-5 {
+  font-size: calc(1.425rem + 2.1vw);
   font-weight: 300;
   line-height: 1.2;
-  font-size: calc(1.425rem + 2.1vw);
 }
 @media (min-width: 1200px) {
   .display-5 {
     font-size: 3rem;
   }
 }
 
 .display-6 {
+  font-size: calc(1.375rem + 1.5vw);
   font-weight: 300;
   line-height: 1.2;
-  font-size: calc(1.375rem + 1.5vw);
 }
 @media (min-width: 1200px) {
   .display-6 {
     font-size: 2.5rem;
   }
\ No newline at end of file
@@ -5504,12 +5504,12 @@
   width: auto;
   margin: var(--bs-modal-margin);
   pointer-events: none;
 }
 .modal.fade .modal-dialog {
-  transform: translate(0, -50px);
   transition: transform 0.3s ease-out;
+  transform: translate(0, -50px);
 }
 @media (prefers-reduced-motion: reduce) {
   .modal.fade .modal-dialog {
     transition: none;
   }
\ No newline at end of file

As it can be seen here, there's no impact as the mixins don't apply values to some siblings CSS rules for the same selector, so the order here is not important and will produce the same CSS bundle in the end.

Target release

This change is retro-compatible as it works with [email protected] too. However, if we want to help folks, we might envisage to merge this PR, and then maybe release a v5.3.4 rather quickly? What do you think @twbs/css-review?

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)
  • (N/A) My change introduces changes to the documentation
  • (N/A) I have updated the documentation accordingly
  • (N/A) I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related issues

Closes #40621

@julien-deramond julien-deramond marked this pull request as ready for review July 10, 2024 16:46
@julien-deramond julien-deramond requested a review from a team as a code owner July 10, 2024 16:46
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👌

Imran-imtiaz48

This comment was marked as off-topic.

@francoisdelage
Copy link

+1

@simPod
Copy link
Contributor

simPod commented Jul 15, 2024

🚢

@XhmikosR
Copy link
Member

@julien-deramond I'll wait until tomorrow for Dependabot. Then this PR should only include the fix.

@XhmikosR XhmikosR changed the title Bump Sass from 1.77.6 to 1.77.7 Fix use of declarations after nested rules (deprecated in Sass 1.77.7) Jul 15, 2024
@julien-deramond julien-deramond removed the request for review from XhmikosR July 15, 2024 21:08
@julien-deramond julien-deramond merged commit 7f8fed4 into main Jul 15, 2024
14 checks passed
@julien-deramond julien-deramond deleted the main-jd-bump-sass-1.77.7 branch July 15, 2024 21:10
@apepper
Copy link

apepper commented Jul 22, 2024

@julien-deramond Thank you for fixing the deprecation issue of sass!

Is there any ETA when bootstrap version 5.3.4 will be released (which would contain this fix)?

@julien-deramond
Copy link
Member Author

Unfortunately, we don't have a precise release date to communicate, as it'll depend on our availability. But it should be in a few days, hopefully 🙏

@jamesleesaunders
Copy link

Unfortunately, we don't have a precise release date to communicate, as it'll depend on our availability. But it should be in a few days, hopefully 🙏

Is there any more news on 5.3.4 ?

@jamesleesaunders
Copy link

jamesleesaunders commented Aug 27, 2024

Unfortunately, we don't have a precise release date to communicate, as it'll depend on our availability. But it should be in a few days, hopefully 🙏

Is there any more news on 5.3.4 ?

Nobody needs it, nobody is in a hurry

To say 'nobody needs it' is a not strictly true? There are a number of people watching this PR and also a number of issues raised for this:

This does only affect people building bootstrap Sass and I appreciate this is small fry in the grand scheme of things but it would be useful to know if/when Bootstrap v5.3.4 will be released as there does not appear to have been a release since Feb-24.

@petschki
Copy link

We would appreciate a release with this fix very much.

@jamesleesaunders
Copy link

Any news on when Release 5.3.4 will be released?

@apepper
Copy link

apepper commented Nov 7, 2024

@julien-deramond Any updates on the release of 5.3.4 which would include this bugfix? At least I (and probably many others) are waiting for it to finally upgrade to the latest sass version.

@julien-deramond
Copy link
Member Author

julien-deramond commented Nov 7, 2024

Unfortunately, I don't know. I'm not the release manager. Waiting for it too :)

@apepper
Copy link

apepper commented Nov 8, 2024

Who is the release manager then?

@julien-deramond
Copy link
Member Author

You can check #40656

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

Successfully merging this pull request may close these issues.

SASS deprecation warnings with version 1.77.7 - Mixed declarations
10 participants