Replace node-sass with dart-sass - #12156
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
Pull request overview
This PR updates the Twenty Nineteen theme’s front-end build pipeline to replace node-sass (LibSass) with sass (Dart Sass), and adjusts theme Sass/PostCSS to keep the generated CSS output stable (fonts/selectors/colors) under the new compiler.
Changes:
- Replace
node-sasswithsassand update build scripts accordingly. - Refactor the
font-family()Sass mixin away from@extend-based generation for non‑latin font fallbacks. - Add a PostCSS step to enforce one selector per line, and hard-code a few color outputs previously produced by deprecated
lighten()/darken().
Reviewed changes
Copilot reviewed 6 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/wp-content/themes/twentynineteen/style-editor.css | Regenerated editor stylesheet output from Dart Sass + updated mixins/formatting. |
| src/wp-content/themes/twentynineteen/sass/variables-site/_colors.scss | Replace deprecated lighten()/darken() outputs with literal hex values. |
| src/wp-content/themes/twentynineteen/sass/site/primary/_comments.scss | Replace deprecated lighten() output with a literal hex value. |
| src/wp-content/themes/twentynineteen/sass/mixins/_mixins-master.scss | Refactor font-family() mixin to inline non‑latin fallbacks instead of @extend. |
| src/wp-content/themes/twentynineteen/print.css | Regenerated/normalized selector formatting in print CSS output. |
| src/wp-content/themes/twentynineteen/postcss.config.js | Add a custom PostCSS plugin to reformat selector lists. |
| src/wp-content/themes/twentynineteen/package.json | Swap dependency to sass and update build scripts to use the Sass CLI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @mixin font-family( $font_family: $font__body ) { | ||
| font-family: $font_family; | ||
| @extend %non-latin-fonts; | ||
| } | ||
|
|
||
| /* Build our non-latin font styles */ | ||
| %non-latin-fonts { | ||
| @each $lang, $font__fallback in $font__fallbacks { | ||
| &:lang(#{$lang}) { |
|
@desrosj is this ready for review? |
sabernhardt
left a comment
sabernhardt
left a comment
There was a problem hiding this comment.
I added some suggestions for replacing the @extend.
| @@ -126,11 +126,7 @@ | |||
| /* Ensure all font family declarations come with non-latin fallbacks */ | |||
| @mixin font-family( $font_family: $font__body ) { | |||
There was a problem hiding this comment.
The changes in this PR increased the main stylesheet file size from about 229KB to about 279KB. To reduce it to about 203KB, the similar languages could be grouped together with :is(:lang(A),:lang(B)) (as in 47925.diff). That still results in rulesets for non-latin fonts interspersed throughout the stylesheet, in 27 places and including the button element twice, but it would be a small reduction overall. Further optimization could be considered later.
Full mixin without extend:
@mixin font-family( $font_family: $font__body ) {
font-family: $font_family;
// Arabic
&:is(:lang(ar),:lang(ary),:lang(azb),:lang(ckb),:lang(fa),:lang(haz),:lang(ps)) {
font-family: Tahoma, Arial, sans-serif;
}
// Cyrillic
&:is(:lang(be),:lang(bg),:lang(kk),:lang(mk),:lang(mn),:lang(ru),:lang(sah),:lang(sr),:lang(tt),:lang(uk)) {
font-family: "Helvetica Neue", Helvetica, "Segoe UI", Arial, sans-serif;
}
// Chinese (Hong Kong)
&:lang(zh-HK) {
font-family: -apple-system, BlinkMacSystemFont, "PingFang HK", "Helvetica Neue", "Microsoft YaHei New", STHeiti Light, sans-serif;
}
// Chinese (Taiwan)
&:lang(zh-TW) {
font-family: -apple-system, BlinkMacSystemFont, "PingFang TC", "Helvetica Neue", "Microsoft YaHei New", STHeiti Light, sans-serif;
}
// Chinese (China)
&:lang(zh-CN) {
font-family: -apple-system, BlinkMacSystemFont, "PingFang SC", "Helvetica Neue", "Microsoft YaHei New", STHeiti Light, sans-serif;
}
// Devanagari and Gujarati
&:is(:lang(bn),:lang(gu),:lang(hi),:lang(mr),:lang(ne)) {
font-family: Arial, sans-serif;
}
// Greek
&:lang(el) {
font-family: "Helvetica Neue", Helvetica, Arial, sans-serif;
}
// Hebrew
&:lang(he) {
font-family: "Arial Hebrew", Arial, sans-serif;
}
// Japanese
&:lang(ja) {
font-family: -apple-system, BlinkMacSystemFont, "Hiragino Sans", Meiryo, "Helvetica Neue", sans-serif;
}
// Korean
&:lang(ko) {
font-family: "Apple SD Gothic Neo", "Malgun Gothic", "Nanum Gothic", Dotum, sans-serif;
}
// Thai
&:lang(th) {
font-family: "Sukhumvit Set", "Helvetica Neue", helvetica, arial, sans-serif;
}
// Vietnamese
&:lang(vi) {
font-family: "Libre Franklin", sans-serif;
}
}
| "autoprefixer": "^10.4.22", | ||
| "chokidar-cli": "^3.0.0", | ||
| "node-sass": "^9.0.0", | ||
| "sass": "^1.83.0", |
There was a problem hiding this comment.
The version is newer now:
| "sass": "^1.83.0", | |
| "sass": "^1.101.0", |
| if (rule.selector.indexOf(',') !== -1) { | ||
| var before = rule.raws.before || ''; | ||
| var indent = before.substring(before.lastIndexOf('\n') + 1); | ||
| rule.selector = rule.selector.split(/,\s*/).join(',\n' + indent); |
There was a problem hiding this comment.
If the mixin groups :lang() selectors, the space could be removed between them so they do not split onto separate lines. I switched * to + so it matches at least one space after the comma.
| rule.selector = rule.selector.split(/,\s*/).join(',\n' + indent); | |
| rule.selector = rule.selector.replace(/, :lang/g, ',:lang').split(/,\s+/).join(',\n' + indent); |
| var before = rule.raws.before || ''; | ||
| var indent = before.substring(before.lastIndexOf('\n') + 1); | ||
| rule.selector = rule.selector.split(/,\s*/).join(',\n' + indent); | ||
| } |
There was a problem hiding this comment.
The ::first-letter pseudo-element seems to be the only unusable selector when appending the :lang() to the end. If I did not miss any other problems, PostCSS could adjust the one.
| } | |
| } | |
| if (rule.selector.indexOf(':not(:focus)::first-letter:') !== -1) { | |
| rule.selector = rule.selector.replace(':not(:focus)::first-letter', '') + ':not(:focus)::first-letter'; | |
| } |
Then blocks/_blocks.scss would need the double colon syntax (which style-editor.scss already has).
&:not(:focus)::first-letter
This replaces
node-sasswithsass(Dart Sass).Looking into the issues raised on the Trac ticket, here is some output from Claude around each one.
This appears to be caused by the redundancy elimination part of Dart Sass related to the use of
@extend.Full output from Claude: Output ignores five of the selectors when it creates font overrides for each non-latin language
This one seems to be caused by
lighten()/darken(), which have been deprecated. I've gone and hard coded the intended hex values to address this for now.Full output from Claude: Why are hexidecimal colors output as `rgb()` and slightly different?
These come from the lighten()/darken() calls the deprecation warnings flagged in _comments.scss:240 and elsewhere. The
math:
Browsers accept fractional rgb() (CSS Color L4), so the rendered color is rgb(0, 80.5, 119) — half a step off the prior
rgb(0, 81, 119). Imperceptible visually, but technically not byte-identical and slightly off from what the designer
presumably intended (#5177).
The right fix is the same as resolving the deprecation: replace lighten()/darken() calls with the intended literal hex
(best — that's what the designer originally meant), or wrap with color.adjust() which is the modern API. As a stopgap
you can wrap with round() to coerce channels back to integers.