chore: updates to support dart sass 3.0.0 - #8048
Conversation
WalkthroughSass deprecation migration modernizing the codebase to use Sass modules and modern APIs. Replaces deprecated global functions with namespaced module equivalents (e.g., Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Preview: https://pf-pr-8048.surge.sh A11y report: https://pf-pr-8048-a11y.surge.sh |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/patternfly/sass-utilities/functions.scss (1)
94-126: Minor cleanup:build-variable-mapcan use$valueinstead ofmap.get($map, $size)(same behavior, clearer).Proposed diff
@function build-variable-map($namespace, $map: ()) { $new-map: (); @each $size, $value in $map { - $new-map: map.merge($new-map, (map.get($map, $size): --#{$namespace}--#{$size})); + $new-map: map.merge($new-map, ($value: --#{$namespace}--#{$size})); } @return $new-map; }src/patternfly/sass-utilities/mixins.scss (1)
300-352: Updatetype-of()tometa.type-of()at lines 89 and 131 to complete the Sass module migration.The code at lines 300-352 correctly uses
list.append()andlist.length(). However, the sweep identified deprecatedtype-of()calls without themeta.module prefix at lines 89 and 131 in the same file. These should be updated tometa.type-of()to align with the modern Sass module syntax used elsewhere in the codebase.
📜 Review details
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/patternfly/assets/fontawesome/_variables.scsssrc/patternfly/base/tokens/tokens-local.scsssrc/patternfly/components/Drawer/drawer.scsssrc/patternfly/components/Sidebar/sidebar.scsssrc/patternfly/layouts/Flex/flex.scsssrc/patternfly/sass-utilities/functions.scsssrc/patternfly/sass-utilities/mixins.scss
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-upload
🔇 Additional comments (15)
src/patternfly/assets/fontawesome/_variables.scss (2)
15-17:fa-content()migration tostring.unquote()looks correct; optionally normalize interpolation spacing.Behavior should remain equivalent to the prior global
unquote()usage; consider#{$fa-var}for consistency/avoiding style churn.Optional diff (no behavior change intended)
@function fa-content($fa-var) { - @return string.unquote("\"#{ $fa-var }\""); + @return string.unquote("\"#{$fa-var}\""); }
3-4: No action needed. The repository specifies Dart Sass 1.74.1 inpackage.json, which fully supports thesass:stringmodule andstring.unquote()function (available since Dart Sass 1.23.0). The code change is compatible with the build toolchain.src/patternfly/base/tokens/tokens-local.scss (1)
1-1:@use ... as *is fine for parity with the old@import, but verify no symbol collisions and module-system compatibility.
This changes load semantics to “load once” and can error on duplicate names that@importmay have tolerated.src/patternfly/components/Sidebar/sidebar.scss (1)
254-269: LGTM: consistent switch tomath.percentage(math.div(...))for width modifiers; verify Sass support.src/patternfly/sass-utilities/functions.scss (3)
5-17: Module-based replacements inpf-strip-unitlook correct; just ensure Sass module APIs are available in your toolchain.
33-46:pf-breakpoint-value/pf-height-breakpoint-value: correctmap.has-key+map.getmigration.
49-92: Breakpoint-map builders: goodlist.length+map.mergemigration; verify output ordering/keys match prior behavior.
(Especially the “base” entry and any “invalid breakpoint …” entries.)src/patternfly/sass-utilities/mixins.scss (1)
1-6: Required import added (sass:list) for the newlist.*calls.src/patternfly/components/Drawer/drawer.scss (1)
704-720: No issues found with themath.percentage(math.div(...))migration.Sass 1.74.1 fully supports
math.percentage()andmath.div()functions (available since Dart Sass 1.23.0+), and the code correctly uses them. No deprecated globalpercentage()calls remain in the codebase.src/patternfly/layouts/Flex/flex.scss (6)
305-314: Spacer variable lookup migration tomap.get(...)is correct.
Keeping interpolation insidevar(#{...})preserves unquoted custom property names.
323-335:map.get(...)updates in.pf-m-spacer-*look consistent with existing behavior.
No semantic changes beyond the Sass API migration.
340-366: Selector list construction:list.append(..., $separator: comma)is the right replacement forappend(...).
This should keep:is(#{$pf-v6-l-flex--gap--selectors})emitting a comma-separated selector list.
368-391: Row-gap selector list migration tolist.appendlooks correct.
393-416: Column-gap selector list migration tolist.appendlooks correct.
1-3: Sass module migration is safe; repo uses Dart Sass 1.74.1 with full@usesupport.The code changes are correct. The repository is pinned to
sass@^1.74.1(Dart Sass), which fully supports the module system,sass:list,sass:map, and namespaced function calls (map.get(),list.append()). The Gulp build system will compile these without issue.
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/patternfly/sass-utilities/functions.scss (1)
120-130: Simplifybuild-variable-map: avoid redundantmap.get($map, $size)inside the loop.
You already have$valuefrom@each $size, $value in $map, so re-fetching it is unnecessary and makes intent harder to read.Proposed refactor
@function build-variable-map($namespace, $map: ()) { $new-map: (); @each $size, $value in $map { - $new-map: map.merge($new-map, (map.get($map, $size): --#{$namespace}--#{$size})); + $new-map: map.merge($new-map, ($value: --#{$namespace}--#{$size})); } @return $new-map; }src/patternfly/sass-utilities/mixins.scss (1)
395-416: Fix the CI stylelint blocker: add an empty line before the@ifinpf-v6-rtl/pf-v6-ltr.
CI reportsat-rule-empty-line-beforeat Lines 397 and 409.Proposed fix
@mixin pf-v6-rtl($hasWhere: true) { $sel: ':is'; + @if $hasWhere { $sel: ':where'; } @at-root #{$sel}(.#{$pf-prefix}m-dir-rtl, [dir="rtl"]) #{&} { @content; } } @mixin pf-v6-ltr($hasWhere: true) { $sel: ':is'; + @if $hasWhere { $sel: ':where'; } @at-root #{$sel}(.#{$pf-prefix}m-dir-ltr, [dir="ltr"]) #{&} { @content; } }
🤖 Fix all issues with AI agents
In @scripts/gulp/sass.mjs:
- Around line 24-27: Replace the manual file:// URL construction used in
sass.compileString (currently new URL(`file://${chunk.history[0]}`)) with Node's
pathToFileURL to handle Windows paths: import pathToFileURL from 'url' at the
top and pass pathToFileURL(chunk.history[0]) as the url option to
sass.compileString so chunk.history[0] is converted correctly across platforms.
In @src/patternfly/components/DescriptionList/description-list-order.scss:
- Around line 9-12: The interpolation for $breakpoint-name is using an empty
variable causing class names like -on-#{}; update the assignment to include the
$breakpoint variable (e.g., $breakpoint-name: -on-#{$breakpoint};) so breakpoint
suffixes (sm/md/…) are appended correctly for modifiers such as
.pf-m-order-1-on-*, and add a blank line before the @if block to satisfy
linting/pipeline requirements; locate the $breakpoint and $breakpoint-name
variables in description-list-order.scss and change only the interpolation and
spacing accordingly.
In @src/patternfly/components/DescriptionList/description-list.scss:
- Around line 198-201: The $breakpoint-name interpolation is missing the
$breakpoint variable causing class names like ".pf-m-1-col-on-"; update the
assignment for $breakpoint-name to include the breakpoint variable (use
-on-#{$breakpoint} instead of -on-#{}) in the description-list.scss snippet,
matching the fix used in pagination.scss so generated class names include the
actual breakpoint (e.g., -on-md).
In @src/patternfly/components/DescriptionList/description-list.scss.bak:
- Around line 1-225: This is a stray backup file (description-list.scss.bak)
that should not be in the PR; remove the file from the branch (e.g., git rm
src/patternfly/components/DescriptionList/description-list.scss.bak && git
commit -m "Remove .bak backup file") ensure nothing in the repo references it
(search for ".bak" or the filename) and push the branch; optionally add a rule
to .gitignore if you want to avoid committing similar backups in future.
In @src/patternfly/components/Drawer/drawer.scss:
- Around line 706-709: Add a blank line before the @if block to satisfy SCSS
lint rules: in the snippet that sets $breakpoint-name and checks @if $breakpoint
!= "base", insert an empty line between the "$breakpoint-name: "";" assignment
and the "@if $breakpoint != "base" {" conditional so the @if statement is
separated from the previous declaration.
- Around line 730-733: Add a blank line before the `@if` directive in the
`$breakpoint-name` block to satisfy linting rules: ensure the snippet that
defines `$breakpoint-name: "";` is followed by an empty line, then the `@if
$breakpoint != "base" { $breakpoint-name: -on-#{$breakpoint}; }` block (this is
the second occurrence—match the same spacing as the first occurrence referencing
`$breakpoint` and `$breakpoint-name`).
In @src/patternfly/components/JumpLinks/jump-links.scss:
- Around line 98-101: The interpolation for $breakpoint-name is missing the
$breakpoint variable and there’s no blank line before the @if at-rule: update
the interpolation to include $breakpoint (e.g., -on-#{$breakpoint}) so
$breakpoint-name becomes correct, and insert a blank line above the @if to
satisfy the at-rule-empty-line-before lint rule; modify the $breakpoint-name
assignment and the @if block in jump-links.scss accordingly.
In @src/patternfly/components/Masthead/masthead.scss:
- Around line 285-288: The SCSS interpolation is empty (using -on-#{}),
producing broken class names; update the assignment of $breakpoint-name to
interpolate the actual $breakpoint (use -on-#{$breakpoint} instead of -on-#{})
and add an empty blank line before the @if block to resolve the linter complaint
so $breakpoint-name becomes correct when $breakpoint != "base".
In @src/patternfly/components/Page/page.scss:
- Around line 567-570: The interpolation for $breakpoint-name is missing the
$breakpoint variable causing names like `-on-#{}`; update the assignment to
include the variable (e.g., change `$breakpoint-name: -on-#{}` to
`$breakpoint-name: -on-#{$breakpoint};`) so class names render correctly, and
ensure the surrounding `@if $breakpoint != "base"` block remains intact (no
extra characters or removed whitespace that would break SASS interpolation).
In @src/patternfly/components/Page/page.scss.bak:
- Around line 566-570: Add a blank line before the @include
pf-v6-apply-breakpoint at-rule inside the @each loop to satisfy the SCSS lint
rule; locate the block beginning with @each $breakpoint, $breakpoint-value in
$pf-page-v6--breakpoint-map and the $breakpoint-name assignment, then insert one
empty line immediately before the line that reads "@include
pf-v6-apply-breakpoint($breakpoint) {" (the comment "// stylelint-disable
max-nesting-depth" can remain where it is).
In @src/patternfly/components/Pagination/pagination.scss:
- Around line 233-236: The interpolation for $breakpoint-name is missing the
$breakpoint variable which produces empty suffixes; update the assignment so
$breakpoint is included in the interpolation (use -on-#{$breakpoint} when
$breakpoint != "base") so $breakpoint-name yields values like -on-md instead of
-on-; adjust the ternary/@if branch that sets $breakpoint-name (and any usages
relying on $breakpoint-name) to use the corrected interpolation.
In @src/patternfly/components/ProgressStepper/progress-stepper.scss:
- Around line 433-436: The SCSS interpolation is empty causing broken modifier
names: replace the empty interpolation in the $breakpoint-name assignment so it
includes the $breakpoint variable (i.e., produce "-on-#{$breakpoint}" instead of
"-on-#{}") or otherwise compute $breakpoint-name from $breakpoint; ensure the
@if block uses that computed $breakpoint-name and add the blank line before the
@if as suggested to satisfy the linter.
In @src/patternfly/components/ProgressStepper/progress-stepper.scss.bak:
- Around line 432-436: The SCSS lint rule requires a blank line before the
at-rule; inside the @each loop where $breakpoint-name is set, add a single empty
line before the @include pf-v6-apply-breakpoint($breakpoint) line so the block
beginning with "@include pf-v6-apply-breakpoint($breakpoint) { ... }" is
separated by one blank line (affecting the scope around $breakpoint-name and the
&.pf-m-horizontal#{$breakpoint-name} selector).
In @src/patternfly/components/Sidebar/sidebar.scss:
- Around line 256-259: The SCSS linter flags a missing empty line before the @if
at-rule in the block that sets $breakpoint-name; open the declaration containing
"$breakpoint-name: "";" and insert a single blank line immediately before the
"@if $breakpoint != \"base\" {" line so the @if is separated by an empty line;
ensure no other whitespace rules are violated around the $breakpoint-name and
@if lines and re-run the SCSS linter to confirm the fix.
In @src/patternfly/components/Tabs/tabs.scss:
- Around line 425-428: The interpolation is missing the `$breakpoint` variable
causing `-on-#{}` to produce broken class names; update the assignment to
compute `$breakpoint-name` using `$breakpoint` (e.g., `$breakpoint-name:
-on-#{$breakpoint};` when `$breakpoint != "base"`) and ensure there is a blank
line inserted before the `@if` to satisfy linting.
- Around line 809-812: The breakpoint name interpolation is missing the
$breakpoint variable (you currently have -on-#{}), which yields empty modifiers;
update the interpolation to include the variable (use -on-#{$breakpoint})
wherever -on-#{} appears (this affects the $breakpoint-name assignment and the
second occurrence noted) so the inset modifier classes correctly include the
breakpoint suffix.
In @src/patternfly/components/Toolbar/toolbar.scss:
- Around line 359-362: The interpolation is empty because $breakpoint wasn't
included; update the $breakpoint-name assignment to interpolate the variable
(use $breakpoint within the interpolation) so it yields -on-#{$breakpoint} when
$breakpoint != "base" and remains an empty string otherwise; edit the block
handling $breakpoint-name and the @if ($breakpoint != "base") condition to use
$breakpoint in the interpolation (refer to $breakpoint-name, $breakpoint and the
@if block) and ensure spacing/blank line formatting is preserved to satisfy the
linter.
In @src/patternfly/layouts/Flex/flex.scss:
- Around line 79-82: SCSS lint fails because there is no blank line before
several @if at-rules; insert a single empty line immediately before each @if
block (e.g., before the @if that computes $breakpoint-name from $breakpoint and
the repeated @if occurrences that follow through the file) so every @if at-rule
is preceded by one blank line to satisfy the style rule; update all noted
occurrences (the @if near the $breakpoint-name assignment and the similar @if
blocks repeated later) consistently.
- Around line 354-371: The SCSS violates lint rules by not having empty lines
before variable declarations and @if blocks; add a blank line immediately before
each $-variable declaration (e.g., before $breakpoint-name, $spacer-name,
$spacer-row, $spacer-column) and before each @if block (e.g., @if $breakpoint ==
"base" and @if $spacer == "base") in the shown @each loop; apply the same
spacing fixes to the comparable blocks that define
$spacer-name/$spacer-row/$spacer-column and their @if checks later in the file
so they conform to scss/dollar-variable-empty-line-before and
at-rule-empty-line-before rules.
📜 Review details
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (26)
scripts/gulp/sass.mjssrc/patternfly/assets/fontawesome/_variables.scsssrc/patternfly/base/tokens/tokens-local.scsssrc/patternfly/components/DescriptionList/description-list-order.scsssrc/patternfly/components/DescriptionList/description-list-order.scss.baksrc/patternfly/components/DescriptionList/description-list.scsssrc/patternfly/components/DescriptionList/description-list.scss.baksrc/patternfly/components/Drawer/drawer.scsssrc/patternfly/components/JumpLinks/jump-links.scsssrc/patternfly/components/JumpLinks/jump-links.scss.baksrc/patternfly/components/Masthead/masthead.scsssrc/patternfly/components/Masthead/masthead.scss.baksrc/patternfly/components/Page/page.scsssrc/patternfly/components/Page/page.scss.baksrc/patternfly/components/Pagination/pagination.scsssrc/patternfly/components/Pagination/pagination.scss.baksrc/patternfly/components/ProgressStepper/progress-stepper.scsssrc/patternfly/components/ProgressStepper/progress-stepper.scss.baksrc/patternfly/components/Sidebar/sidebar.scsssrc/patternfly/components/Tabs/tabs.scsssrc/patternfly/components/Tabs/tabs.scss.baksrc/patternfly/components/Toolbar/toolbar.scsssrc/patternfly/components/Toolbar/toolbar.scss.baksrc/patternfly/layouts/Flex/flex.scsssrc/patternfly/sass-utilities/functions.scsssrc/patternfly/sass-utilities/mixins.scss
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T15:25:08.994Z
Learnt from: srambach
Repo: patternfly/patternfly PR: 8030
File: src/patternfly/components/TreeView/tree-view.scss:441-445
Timestamp: 2025-12-17T15:25:08.994Z
Learning: In PatternFly TreeView component (src/patternfly/components/TreeView/tree-view.scss), the `.pf-m-disabled` modifier on list items affects only the visual appearance of node content (text and icons), but intentionally does not change the toggle button color. The toggle remains functional and visually distinct to indicate the item can still be expanded/collapsed.
Applied to files:
src/patternfly/components/JumpLinks/jump-links.scss.baksrc/patternfly/components/Tabs/tabs.scss.bak
🪛 GitHub Actions: pr-preview
src/patternfly/components/Pagination/pagination.scss.bak
[error] 234-234: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/DescriptionList/description-list-order.scss.bak
[error] 10-10: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/DescriptionList/description-list.scss
[error] 199-199: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/DescriptionList/description-list-order.scss
[error] 10-10: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/JumpLinks/jump-links.scss.bak
[error] 99-99: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/Toolbar/toolbar.scss
[error] 360-360: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/Masthead/masthead.scss.bak
[error] 286-286: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/JumpLinks/jump-links.scss
[error] 99-99: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/Tabs/tabs.scss.bak
[error] 426-426: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 810-810: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/Drawer/drawer.scss
[error] 707-707: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 731-731: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/ProgressStepper/progress-stepper.scss
[error] 434-434: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/DescriptionList/description-list.scss.bak
[error] 199-199: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/Masthead/masthead.scss
[error] 286-286: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/Page/page.scss.bak
[error] 568-568: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/sass-utilities/mixins.scss
[error] 397-397: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 409-409: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/ProgressStepper/progress-stepper.scss.bak
[error] 434-434: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/Tabs/tabs.scss
[error] 426-426: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 810-810: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/Page/page.scss
[error] 568-568: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/layouts/Flex/flex.scss
[error] 80-80: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 310-310: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 332-332: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 356-356: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 359-359: SCSS Lint: 'scss/dollar-variable-empty-line-before' rule violation. Expected empty line before $-variable.
[error] 360-360: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 363-363: SCSS Lint: 'scss/dollar-variable-empty-line-before' rule violation. Expected empty line before $-variable.
[error] 364-364: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 367-367: SCSS Lint: 'scss/dollar-variable-empty-line-before' rule violation. Expected empty line before $-variable.
[error] 368-368: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 371-371: SCSS Lint: 'scss/dollar-variable-empty-line-before' rule violation. Expected empty line before $-variable.
[error] 396-396: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 399-399: SCSS Lint: 'scss/dollar-variable-empty-line-before' rule violation. Expected empty line before $-variable.
[error] 400-400: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 403-403: SCSS Lint: 'scss/dollar-variable-empty-line-before' rule violation. Expected empty line before $-variable.
[error] 404-404: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 407-407: SCSS Lint: 'scss/dollar-variable-empty-line-before' rule violation. Expected empty line before $-variable.
[error] 430-430: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 433-433: SCSS Lint: 'scss/dollar-variable-empty-line-before' rule violation. Expected empty line before $-variable.
[error] 434-434: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 437-437: SCSS Lint: 'scss/dollar-variable-empty-line-before' rule violation. Expected empty line before $-variable.
[error] 438-438: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
[error] 441-441: SCSS Lint: 'scss/dollar-variable-empty-line-before' rule violation. Expected empty line before $-variable.
src/patternfly/components/Toolbar/toolbar.scss.bak
[error] 360-360: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/Sidebar/sidebar.scss
[error] 257-257: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
src/patternfly/components/Pagination/pagination.scss
[error] 234-234: SCSS Lint: 'at-rule-empty-line-before' rule violation. Expected empty line before at-rule.
🔇 Additional comments (16)
src/patternfly/assets/fontawesome/_variables.scss (1)
4-4: LGTM! Correct migration to Sass module system.The addition of
@use 'sass:string';and the migration fromunquote()tostring.unquote()correctly align with Dart Sass 3.0.0 requirements, which deprecate global functions in favor of the module system. The function behavior remains identical, and this change is consistent with the existingsass:mathmodule usage in the file.Also applies to: 16-16
scripts/gulp/sass.mjs (2)
28-28: LGTM: Correct handling for new Sass API.The change from
css.css.toString()tocss.cssis correct. ThecompileStringAPI returns aCompileResultwhere thecssproperty is already a string, eliminating the need for explicit conversion.
24-27: The current implementation correctly uses the Dart Sass 3.0.0 API for import resolution.The
compileStringcall with afile:URL parameter automatically uses aFilesystemImporterby default, which handles relative import resolution correctly. According to Dart Sass documentation, whenurlis provided as a file URL without a customimporteroption, the filesystem importer resolves relative@importand@usestatements based on that URL's canonical location. TheincludePathsand customimportersare optional and only needed for non-standard import paths; they are not required for standard relative imports.Likely an incorrect or invalid review comment.
src/patternfly/base/tokens/tokens-local.scss (1)
1-1:@use ... as *is fine here (matches prior global@importbehavior).
No functional concerns in this file given the previous@importwas also unnamespaced.src/patternfly/sass-utilities/functions.scss (1)
6-17: Good migration to Sass module APIs (meta.*,list.*,map.*).
This is the right direction for Dart Sass module compatibility, andpf-strip-unit’smeta.type-ofcheck looks correct.src/patternfly/sass-utilities/mixins.scss (1)
5-7: Sass module migration looks correct; please verify Dart Sass 3.0.0 compatibility for the specific module APIs used.
Thesass:list/sass:metachanges are consistent with the PR’s module migration.Also applies to: 86-153
src/patternfly/components/Toolbar/toolbar.scss.bak (1)
1-12: File path is incorrect; proposed fix already implemented in codebase.The referenced file
src/patternfly/components/Toolbar/toolbar.scss.bakdoes not exist. The actual file issrc/patternfly/components/Toolbar/toolbar.scss. More importantly, lines 359–360 in the current code already use the explicit@ifapproach you're proposing:$breakpoint-name: ""; @if $breakpoint != "base" { $breakpoint-name: -on-#{}; }The
if()function replacement you suggested is already in place. Verify the correct file path and confirm whether there are any remaining stylelint issues in the current code.Likely an incorrect or invalid review comment.
src/patternfly/components/DescriptionList/description-list-order.scss.bak (1)
1-27: The file referenced (description-list-order.scss.bak) does not exist in the repository. The actual file issrc/patternfly/components/DescriptionList/description-list-order.scss, which already uses@ifsyntax (not the deprecatedif()function shown in the review snippet). Additionally, line 11 of the actual file contains a bug:$breakpoint-name: -on-#{};has an empty interpolation and should be$breakpoint-name: -on-#{$breakpoint};.Likely an incorrect or invalid review comment.
src/patternfly/components/Pagination/pagination.scss.bak (1)
1-5: Fix the empty interpolation in$breakpoint-nameassignment; verify file path reference.The file referenced as
pagination.scss.bakdoes not exist in the repository. The actual file issrc/patternfly/components/Pagination/pagination.scss.Additionally, the code at lines 230-255 in the actual file already uses the
@ifpattern you recommended, and an empty line before the@includealready exists. However, there is a real issue present: line 234 contains$breakpoint-name: -on-#{};with an empty interpolation that appears to be incomplete. This should likely be$breakpoint-name: -on-#{$breakpoint};to properly include the breakpoint name.Likely an incorrect or invalid review comment.
src/patternfly/components/Masthead/masthead.scss.bak (1)
78-109: File location error: The referenced.scss.bakfile does not exist.The mixins
pf-v6-c-masthead--m-display-stackandpf-v6-c-masthead--m-display-inlineare already properly defined in the canonical stylesheetsrc/patternfly/components/Masthead/masthead.scssat lines 78–109, not in a.bakfile. Additionally, the breakpoint loop at lines 283–291 already uses the@ifpattern with proper blank line spacing before@include, exactly as proposed in this review's suggested fix. No changes are needed.Likely an incorrect or invalid review comment.
src/patternfly/layouts/Flex/flex.scss (2)
2-3: LGTM! Correct Sass module imports for Dart Sass 3.0.0.The addition of
sass:listandsass:mapmodule imports aligns with the migration away from deprecated global Sass APIs.
318-318: LGTM! Correct usage ofmap.getandlist.append.The migration from global
map-get/appendto module-namespacedmap.get/list.appendis correct for Dart Sass 3.0.0 compatibility.Also applies to: 339-339, 342-342, 372-372, 408-408, 442-442
src/patternfly/components/Sidebar/sidebar.scss (1)
267-267: LGTM! Correct usage ofmath.percentage.The migration from global
percentage()tomath.percentage()is correct for Dart Sass 3.0.0 compatibility.src/patternfly/components/Tabs/tabs.scss.bak (2)
805-809: Fix SCSS lint violation: add empty line before@include.Same lint violation as above - missing empty line before the
@includeat-rule.Proposed fix
@each $breakpoint, $breakpoint-value in $pf-v6-c-tabs--breakpoint-map { $breakpoint-name: if($breakpoint != "base", -on-#{$breakpoint}, ""); + @include pf-v6-apply-breakpoint($breakpoint) {Likely an incorrect or invalid review comment.
424-428: Fix SCSS lint violation: add empty line before@include.The pipeline is failing due to a missing empty line before the
@includeat-rule. Per theat-rule-empty-line-beforerule, an empty line is expected before at-rules.Proposed fix
@each $breakpoint, $breakpoint-value in $pf-v6-c-tabs--breakpoint-map { $breakpoint-name: if($breakpoint != "base", -on-#{$breakpoint}, ""); + @include pf-v6-apply-breakpoint($breakpoint) {Likely an incorrect or invalid review comment.
src/patternfly/components/Drawer/drawer.scss (1)
718-718: Approve: Correct usage ofmath.percentagewith Sass module syntax.The migration from the global
percentage()function tomath.percentage(math.div(...))aligns with Dart Sass 3.0.0's module-based API requirements.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/patternfly/sass-utilities/mixins.scss (2)
86-153: pf-v6-utility-builder refactor is correct; consider standardizingmeta.type-of()comparisons for readability.
Usingmeta.type-of($props) == map/== listis valid, but it’s inconsistent with quoted comparisons elsewhere in the repo changes.
459-466: Harden pf-root to avoid accidental..fooselectors when callers pass a dotted selector.
Right now,$isClass: truewill blindly prefix".". Consider guarding withstring.index($selector, '.') == 1(or documenting call expectations very explicitly).
🤖 Fix all issues with AI agents
In @src/patternfly/sass-utilities/functions.scss:
- Around line 6-13: The type check in pf-strip-unit uses a quoted type
('number') while other modules use unquoted identifiers; update the
meta.type-of($num) comparison in the pf-strip-unit function to use the unquoted
identifier (number) for consistency (and scan other files for similar quoted vs
unquoted type checks such as map and list to standardize them too).
- Around line 35-49: The pf-height-breakpoint-value and the corresponding
breakpoint lookup function currently return false for unknown keys which can
produce invalid CSS like @media (min-width: false); update both
pf-height-breakpoint-value and the other breakpoint lookup (the function above
it) to throw a Sass @error with a clear message when map.has-key(...) is false
so callers never receive false as a breakpoint value; include the missing key
name and the map variable in the error text to aid debugging.
- Around line 125-127: In build-variable-map's loop you iterate with "@each
$size, $value in $map" but then call map.get($map, $size) redundantly; replace
that lookup with the already-available $value when constructing $new-map (i.e.,
use $value instead of map.get($map, $size)), keeping the rest of the expression
that builds the CSS custom property name (--#{$namespace}--#{$size}) and the
map.merge into $new-map.
📜 Review details
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
backstop_data/bitmaps_reference/pf-core__components_input-group_html_variations_0_document_1_tablet.pngis excluded by!**/*.png
📒 Files selected for processing (17)
scripts/gulp/sass.mjssrc/patternfly/assets/fontawesome/_variables.scsssrc/patternfly/base/tokens/tokens-local.scsssrc/patternfly/components/DescriptionList/description-list-order.scsssrc/patternfly/components/DescriptionList/description-list.scsssrc/patternfly/components/Drawer/drawer.scsssrc/patternfly/components/JumpLinks/jump-links.scsssrc/patternfly/components/Masthead/masthead.scsssrc/patternfly/components/Page/page.scsssrc/patternfly/components/Pagination/pagination.scsssrc/patternfly/components/ProgressStepper/progress-stepper.scsssrc/patternfly/components/Sidebar/sidebar.scsssrc/patternfly/components/Tabs/tabs.scsssrc/patternfly/components/Toolbar/toolbar.scsssrc/patternfly/layouts/Flex/flex.scsssrc/patternfly/sass-utilities/functions.scsssrc/patternfly/sass-utilities/mixins.scss
🔇 Additional comments (30)
src/patternfly/base/tokens/tokens-local.scss (1)
1-1: LGTM! Correct migration to Sass module system.The change from
@importto@use '../../sass-utilities/init' as *;correctly adopts the modern Sass module system, addressing Dart Sass 3.0.0 deprecation warnings. Theas *syntax ensures all exports remain available without namespace prefixes, maintaining compatibility with the rest of the file.src/patternfly/assets/fontawesome/_variables.scss (1)
4-4: LGTM! Correct migration to Sass string module.The addition of
@use 'sass:string';and the change from the deprecated globalunquote()to the namespacedstring.unquote()correctly modernizes the code for Dart Sass 3.0.0 compatibility. The function behavior remains unchanged.Also applies to: 16-16
src/patternfly/components/DescriptionList/description-list-order.scss (1)
9-13: LGTM! Correct refactoring to address Sass 3.0.0 deprecation.The explicit initialization of
$breakpoint-nameto""followed by a conditional assignment replaces the deprecated inlineif()pattern. This produces identical output while addressing Dart Sass 3.0.0 deprecation warnings around conditional expressions used for property construction.src/patternfly/components/Sidebar/sidebar.scss (2)
256-260: LGTM! Consistent pattern for Sass 3.0.0 compatibility.The explicit breakpoint-name initialization and conditional assignment pattern correctly addresses deprecated
if()usage, matching the approach used consistently across other components in this PR.
268-268: LGTM! Correct migration to Sass math module.The change from the deprecated global
percentage()to the namespacedmath.percentage()correctly modernizes the code for Dart Sass 3.0.0. The requiredsass:mathmodule is properly imported at line 1.scripts/gulp/sass.mjs (1)
24-28: LGTM! Correct migration to modern Sass compiler API.The change from the deprecated
sass.renderSync()tosass.compileString()correctly adopts the modern Dart Sass API. Key improvements:
- The
urloption withfile://protocol preserves import resolution semantics- Direct access to
css.cssreflects that the new API returns a string (not a buffer)- This addresses compatibility with Dart Sass 3.0.0
src/patternfly/components/JumpLinks/jump-links.scss (1)
97-103: Breakpoint suffix refactor is safe and should preserve selector output.
This avoids deprecated inline conditional usage while keepingpf-m-expandable#{$breakpoint-name}stable across base/non-base breakpoints.src/patternfly/components/Pagination/pagination.scss (1)
232-238: Breakpoint-name construction is behavior-preserving and consistent with the PR migration.src/patternfly/components/ProgressStepper/progress-stepper.scss (1)
432-438: Breakpoint suffix refactor should not change emitted selectors.src/patternfly/sass-utilities/functions.scss (3)
56-69: build-breakpoint-map modernization is good; watch for “invalid breakpoint …” keys escaping into downstream @each loops.
If any consumer iterates this map without filtering, those diagnostic keys could create unintended selectors.
79-93: build-height-breakpoint-map modernization is good; same diagnostic-key caveat as width breakpoints.
103-115: build-spacer-map migration is fine; verifymap.remove($map, "auto", "0")matches actual key types.
If the spacer map uses numeric0rather than string"0", this won’t remove it.src/patternfly/sass-utilities/mixins.scss (4)
5-7: Module imports are appropriate for Dart Sass 3.0.0 migration.
301-334: Variable-stack builder migration tolist.*is correct; ensure list order still matches breakpoint precedence.
The logic relies on stable iteration order of$breakpoint-map.
384-418: RTL/LTR selector construction refactor is fine and avoids deprecated inline conditional patterns.
450-457: pf-v6-set-inverse is clearer and avoids deprecated inline conditionals.src/patternfly/layouts/Flex/flex.scss (3)
2-3: LGTM!The Sass module imports for
sass:listandsass:mapare correctly added to support the modern namespaced function calls required by Dart Sass 3.0.0.
79-83: LGTM!The breakpoint-name computation is correctly refactored from the deprecated inline
if()syntax to explicit initialization with a conditional assignment. This pattern preserves the original behavior while being compatible with Dart Sass 3.0.0.
358-383: LGTM!The gap modifier logic correctly uses the explicit conditional pattern for
$breakpoint-name,$spacer-name,$spacer-row, and$spacer-column. Thelist.appendcall on line 383 properly uses the namespaced module function.src/patternfly/components/DescriptionList/description-list.scss (1)
198-202: LGTM!The breakpoint-name computation follows the same explicit conditional pattern used consistently across the codebase for Dart Sass 3.0.0 compatibility.
src/patternfly/components/Toolbar/toolbar.scss (1)
359-363: LGTM!The breakpoint-name refactoring is consistent with the pattern applied across all components in this PR.
src/patternfly/components/Masthead/masthead.scss (1)
285-289: LGTM!The breakpoint-name computation is correctly refactored for Dart Sass 3.0.0 compatibility.
src/patternfly/components/Tabs/tabs.scss (2)
425-429: LGTM!The breakpoint-name computation within the vertical tabs expandable/non-expandable modifiers loop is correctly refactored.
810-814: LGTM!The breakpoint-name computation for the inset spacer modifiers loop follows the consistent pattern.
src/patternfly/components/Drawer/drawer.scss (3)
1-2: LGTM!The
sass:mathmodule import is correctly added alongside the existing sass-utilities import to support themath.percentage()andmath.div()functions.
706-719: LGTM!The width modifier loop correctly uses:
- Explicit conditional for
$breakpoint-nameinitializationmath.percentage(math.div($width-value, 100))replacing the deprecatedpercentage($width-value / 100)syntaxThe calculation produces identical results using the modern Sass math module API.
731-735: LGTM!The breakpoint-name computation for responsive modifiers follows the consistent pattern.
src/patternfly/components/Page/page.scss (3)
404-408: LGTM!The breakpoint-name computation for height-based sticky modifiers correctly uses the
-on-#{$breakpoint}-heightsuffix pattern with explicit conditional assignment.
523-527: LGTM!The breadcrumb sticky modifier height breakpoint follows the same pattern as the first height breakpoint loop.
575-579: LGTM!The padding modifier breakpoint loop uses the standard
-on-#{$breakpoint}suffix pattern, consistent with other non-height-based breakpoint loops across the codebase.
| @use 'sass:meta'; | ||
| @use 'sass:list'; | ||
| @use 'sass:map'; | ||
|
|
||
| // Transform px to rems | ||
| @function pf-strip-unit($num) { | ||
| @if type-of($num) == 'number' and not unitless($num) { | ||
| @if meta.type-of($num) == 'number' and not math.is-unitless($num) { | ||
| @return math.div($num, ($num * 0 + 1)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Sass module migration looks correct; keep type checks consistent.
You’re using quoted types here ('number') while other files compare against unquoted identifiers (map, list). It all works, but consider standardizing for readability.
🤖 Prompt for AI Agents
In @src/patternfly/sass-utilities/functions.scss around lines 6 - 13, The type
check in pf-strip-unit uses a quoted type ('number') while other modules use
unquoted identifiers; update the meta.type-of($num) comparison in the
pf-strip-unit function to use the unquoted identifier (number) for consistency
(and scan other files for similar quoted vs unquoted type checks such as map and
list to standardize them too).
| @if map.has-key($breakpoint-map, #{$breakpoint}) { | ||
| @return #{map.get($breakpoint-map, #{$breakpoint})}; | ||
| } @else { | ||
| @return false; | ||
| } | ||
| } | ||
|
|
||
| // Return height breakpoint value if it exists | ||
| @function pf-height-breakpoint-value($height-breakpoint, $height-breakpoint-map: $pf-v6-global--height-breakpoint-name-map) { | ||
| $height-breakpoint-value: if(map-has-key($height-breakpoint-map, #{$height-breakpoint}), map-get($height-breakpoint-map, #{$height-breakpoint}), false); | ||
|
|
||
| @return #{$height-breakpoint-value}; | ||
| @if map.has-key($height-breakpoint-map, #{$height-breakpoint}) { | ||
| @return #{map.get($height-breakpoint-map, #{$height-breakpoint})}; | ||
| } @else { | ||
| @return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Map API migration for breakpoint lookups is correct, but ensure callers never consume false as a breakpoint.
If an invalid breakpoint name slips through, pf-v6-apply-breakpoint() will emit @media (min-width: false); if that’s intended legacy behavior, fine—otherwise consider an @error in a follow-up.
🤖 Prompt for AI Agents
In @src/patternfly/sass-utilities/functions.scss around lines 35 - 49, The
pf-height-breakpoint-value and the corresponding breakpoint lookup function
currently return false for unknown keys which can produce invalid CSS like
@media (min-width: false); update both pf-height-breakpoint-value and the other
breakpoint lookup (the function above it) to throw a Sass @error with a clear
message when map.has-key(...) is false so callers never receive false as a
breakpoint value; include the missing key name and the map variable in the error
text to aid debugging.
| @each $size, $value in $map { | ||
| $new-map: map-merge($new-map, (map-get($map, $size): --#{$namespace}--#{$size})); | ||
| $new-map: map.merge($new-map, (map.get($map, $size): --#{$namespace}--#{$size})); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove redundant map lookups in build-variable-map (use the iterated $value).
Right now you compute the value via @each $size, $value in $map but then fetch it again with map.get($map, $size).
Proposed diff
@function build-variable-map($namespace, $map: ()) {
$new-map: ();
@each $size, $value in $map {
- $new-map: map.merge($new-map, (map.get($map, $size): --#{$namespace}--#{$size}));
+ $new-map: map.merge($new-map, ($value: --#{$namespace}--#{$size}));
}
@return $new-map;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @each $size, $value in $map { | |
| $new-map: map-merge($new-map, (map-get($map, $size): --#{$namespace}--#{$size})); | |
| $new-map: map.merge($new-map, (map.get($map, $size): --#{$namespace}--#{$size})); | |
| } | |
| @function build-variable-map($namespace, $map: ()) { | |
| $new-map: (); | |
| @each $size, $value in $map { | |
| $new-map: map.merge($new-map, ($value: --#{$namespace}--#{$size})); | |
| } | |
| @return $new-map; | |
| } |
🤖 Prompt for AI Agents
In @src/patternfly/sass-utilities/functions.scss around lines 125 - 127, In
build-variable-map's loop you iterate with "@each $size, $value in $map" but
then call map.get($map, $size) redundantly; replace that lookup with the
already-available $value when constructing $new-map (i.e., use $value instead of
map.get($map, $size)), keeping the rest of the expression that builds the CSS
custom property name (--#{$namespace}--#{$size}) and the map.merge into
$new-map.
|
🎉 This PR is included in version 6.5.0-prerelease.34 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fixes #8045
This basically updates our use of sass to address deprecation warnings for the dart sass 3.0.0 release.
To validate I
diffed thedistdir between this branch andmainand neitherpatternfly.cssorpatternfly-addons.css(utilities) changed. The sass did, as expected.I also re-ran visual regressions and got one diff, but it's from a screenshot I goofed up and didn't check in the last PR that updated screenshots. I added that missing screenshot to this PR, too. Feel free to ignore it.
Dev server seems to run fine.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.