-
-
Notifications
You must be signed in to change notification settings - Fork 78.9k
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
Drop node-sass
support
#41112
base: main
Are you sure you want to change the base?
Drop node-sass
support
#41112
Conversation
898c14a
to
1816145
Compare
1816145
to
f232df5
Compare
@@ -46,7 +46,7 @@ | |||
} | |||
} | |||
@include color-mode(dark, true) { | |||
--custom-color: #{mix($indigo, $blue, 50%)}; |
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 removed the mix($indigo, $blue, 50%)
because color.mix($indigo, $blue, 50%)
doesn't return a value as an hex string, but a color that should be transformed to a hex string. I don't think it's necessary to add such complexity for now. Let's see at the end of the PR if we reintroduce something like that.
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 think that the mix($indigo, $blue, 50%) is not appropriate in this as css and bootstrap has feature of colour-mix which include the change or mix in colour using hex string but you can't use colour.mix to change colour into hex string
906f9b8
to
8ec183e
Compare
8ec183e
to
343bc9e
Compare
@@ -14,23 +14,23 @@ | |||
}, | |||
{ | |||
"path": "./dist/css/bootstrap-reboot.min.css", | |||
"maxSize": "3.25 kB" | |||
"maxSize": "3.5 kB" |
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.
Changes here are for now due to the introduction of color.*
functions that return the color as a rgb values instead of an hex value. The strings for the fill
values of SVGs are a bit longer, so the bundle is a little bit bigger. For instance:
4698,4699c4698,4699
< --bs-accordion-btn-icon: url("data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16' fill='%236ea8fe'%3e%3cpath fill-rule='evenodd' d='M1.646 4.646a.5.5 0 0 1 .708 0L8 10.293l5.646-5.647a.5.5 0 0 1 .708.708l-6 6a.5.5 0 0 1-.708 0l-6-6a.5.5 0 0 1 0-.708'/%3e%3c/svg%3e");
< --bs-accordion-btn-active-icon: url("data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16' fill='%236ea8fe'%3e%3cpath fill-rule='evenodd' d='M1.646 4.646a.5.5 0 0 1 .708 0L8 10.293l5.646-5.647a.5.5 0 0 1 .708.708l-6 6a.5.5 0 0 1-.708 0l-6-6a.5.5 0 0 1 0-.708'/%3e%3c/svg%3e");
---
> --bs-accordion-btn-icon: url("data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16' fill='rgb%28109.8, 168, 253.8%29'%3e%3cpath fill-rule='evenodd' d='M1.646 4.646a.5.5 0 0 1 .708 0L8 10.293l5.646-5.647a.5.5 0 0 1 .708.708l-6 6a.5.5 0 0 1-.708 0l-6-6a.5.5 0 0 1 0-.708'/%3e%3c/svg%3e");
> --bs-accordion-btn-active-icon: url("data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16' fill='rgb%28109.8, 168, 253.8%29'%3e%3cpath fill-rule='evenodd' d='M1.646 4.646a.5.5 0 0 1 .708 0L8 10.293l5.646-5.647a.5.5 0 0 1 .708.708l-6 6a.5.5 0 0 1-.708 0l-6-6a.5.5 0 0 1 0-.708'/%3e%3c/svg%3e");
However, it doesn't seem to be a regression so far.
@julien-deramond Sass color functions don't round particular values in RGB colors (Please check this - sass/dart-sass#2456). You chose to round values in the following functions:
I recommend avoiding math rounds in those functions, as suggested by the Sass creators. It requires modifying the luminance function like this. The
|
Warning
This PR is heavily draft. It prepares the modifications for when we'll drop
node-sass
supportContext
Sass has begun deprecating certain features (see #40962 and #40849), which currently generate warning messages during compilation with Sass.
The first major breaking change for us came in Sass 1.79.x, where the
blue()
,red()
, andgreen()
functions were deprecated. These functions were replaced by thecolor.channel()
function, which is not supported bynode-sass
, and where we can't use any workaround on our side.As a result, addressing these warnings requires us to drop
node-sass
support really soon.Bootstrap v6 development has not yet started, which raises an important consideration: Dart Sass v2 might be released before Bootstrap v6. If that happens, it may become necessary to remove
node-sass
support in the final Bootstrap v5.x release rather than waiting for v6. This is because Bootstrap v6 will likely introduce a significant set of new features, whereas a v5.x release can focus on resolving compatibility issues. Droppingnode-sass
in the last v5.x release ensures that all v5 content remains accessible tonode-sass
users while aligning with Dart Sass's latest updates.Even if it is clearly a breaking change, for
node-sass
users, with the right communication, I think this could be an acceptable solution for everyone.It'll also mean that for everyone, cascading the
node-sass
removal, Bootstrap users will need to use@use
instead of@import
, but also@use <color;math;map;etc.>
.Description
This PR drops
node-sass
support and bump thesass
dependency to the latest release.Following these changes, users will need to use
@use
instead of@import
, and use@use
Sass color, math, map, etc. modules.Sub-tasks
node-sass
GitHub workflowRGBA
withrgba
sass
→ 1.79.6Deprecation Warning: blue() is deprecated. Suggestion: color.channel($color, "blue", $space: rgb) More info: https://sass-lang.com/d/color-functions
(same thing forred()
andgreen()
) → 1816145sass
→ 1.80.xsass
→ 1.81.xsass
→ 1.82.xsass
→ 1.83.xdist/css/bootstrap.css
from the main branch and this onerfs
repository (andvendor
dir here)docs
job, and with Netlifynode-sass
specific content (if any)node-sass
anymoreDart Sass and Hugo
However, by default, Hugo will still use "libsass". Based on the official Hugo doc for Dart Sass, I've made the following changes:
sass
dependency bysass-embedded
because Hugo needs the embedded version to compile and run the documentation. If we have kept thesass
dependency, we should install it globally on the computer instead of using the one innode_modules/.bin
Please note that it can be considered as safe since it's mentioned in https://github.com/sass/embedded-host-node that:
hugo-bin"
parameter inpackage.json
"transpiler" "dartsass"
in Sass options insite/layouts/partials/stylesheet.html
sudo snap install dart-sass
in.github/workflows/docs.yml
Please note that there was a test PR made a long time ago at #39700 too.
Type of changes
Checklist
npm run lint
)Live previews
Related issues
Closes #40962
Closes #40849