Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces opacity-based glass tokens with explicit color tokens, expands glass-theme token sets, adjusts box-shadow/border/brand-accent mappings, applies glass/plain styling across components, adds a page sidebar wrapper, and updates demos and timestamps. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (1 passed)
📝 Coding Plan
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 |
Collaborator
|
Preview: https://pf-pr-8207.surge.sh A11y report: https://pf-pr-8207-a11y.surge.sh |
8000
mcoker
force-pushed
the
glass-hack-3.5.26
branch
from
March 10, 2026 20:58
3cb06a4 to
cc34699
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/patternfly/demos/Card/card-template-gallery.hbs (1)
15-15: Consider whether the CSS TODO forpf-m-secondaryborders should be addressed alongside this change.The
pf-m-secondarymodifier currently applies only background color—border styling is commented out with a TODO ("let selectable secondary cards have a border"). This means the card-1 demo will show selectable styling with reduced visual feedback. If this is intentional for the glass theme WIP, no action needed; otherwise, address the CSS limitation incard.scss:233-234.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patternfly/demos/Card/card-template-gallery.hbs` at line 15, The demo uses the pf-m-secondary modifier on the selectable card (card--id built from card-template-gallery--id and card--modifier="pf-m-selectable-raised pf-m-secondary pf-m-compact") but the CSS TODO in the card styles leaves secondary cards without a border; decide whether this is intentional for the glass theme WIP—if not, update the card SCSS rule that targets .pf-m-secondary (and the selectable variant .pf-m-selectable-raised .pf-m-secondary) to add the intended border styles (undo the commented TODO lines around the current card.scss border rules) so the card-1 demo shows full selectable visual feedback; if intentional, add a brief inline comment near the markup (card-template-gallery--id / card--modifier usage) stating the missing border is deliberate to avoid future churn.src/patternfly/patternfly.scss (1)
8-17: POC artifacts should be cleaned before merge.The comment "custom stuff for the POC" and commented-out localhost URLs indicate this is proof-of-concept code. Consider removing debug artifacts before merging:
:where(.pf-v6-theme-glass) { - // custom stuff for the POC .pf-v6-c-page, .pf-v6-c-login, body { - // background-image: url(http://localhost:8001/assets/images/glass-brand-light.jpg); background-image: url("./assets/images/compass--wallpaper-light.jpg");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patternfly/patternfly.scss` around lines 8 - 17, Remove the POC artifacts in the .pf-v6-theme-glass block: delete the inline comment "// custom stuff for the POC" and remove the commented-out localhost background-image line so only the intended production asset remains; confirm the selectors .pf-v6-c-page, .pf-v6-c-login, and body keep a single clean background-image rule (or move the image path to a configuration variable) and ensure no localhost/debug URLs or commented debug lines remain before merging.src/patternfly/components/Drawer/drawer.scss (1)
862-865: Remove empty placeholder comment block.This commented-out empty block should be removed before merge:
-// :where(.pf-v6-theme-glass) { - -// }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patternfly/components/Drawer/drawer.scss` around lines 862 - 865, Remove the empty commented placeholder block containing ":where(.pf-v6-theme-glass)" in drawer.scss: delete the three-line commented section (// :where(.pf-v6-theme-glass) { // } ) so no stray empty comment remains in the Drawer styles.src/patternfly/components/Page/page.scss (1)
316-350: Consider removing commented-out selector.Line 318 has a commented-out alternative selector
// &:has(> .#{$page}__sidebar-main). If this is no longer needed, consider removing it to avoid confusion. If it's intentionally kept for future reference or as WIP, a brief comment explaining why would be helpful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patternfly/components/Page/page.scss` around lines 316 - 350, Remove the leftover commented selector "// &:has(> .#{$page}__sidebar-main)" in the `@at-root` :where(.pf-v6-theme-glass) block of page.scss to avoid confusion, or if you intend to keep it as a deliberate note, replace it with a short explanatory comment indicating why the &:has(...) variant is preserved for future use; reference the surrounding selectors .pf-v6-theme-glass and .#{$page}__sidebar-main so reviewers can locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patternfly/components/Card/card.scss`:
- Around line 231-235: The .pf-m-secondary block currently comments out the
border token assignments causing all secondary cards to lose their secondary
border; restore the two assignments (--#{$card}--BorderColor and
--#{$card}--BorderWidth) inside the .pf-m-secondary rule so that non-selectable
secondary cards keep their intended border tokens, and if selectable/clickable
secondary cards should differ, add a narrower override on the combined selector
(e.g., the .pf-m-secondary selector combined with the selectable/clickable
modifier such as .pf-m-selectable or .pf-m-clickable) to change those border
tokens only for that combined state rather than removing them from
.pf-m-secondary itself.
In `@src/patternfly/components/Masthead/masthead.scss`:
- Around line 214-220: The glass-theme override is forcing background-color on
the masthead, bypassing the masthead background CSS variable used by
.#{$masthead}__expandable-content; replace the direct background-color
assignment inside the `@at-root` :where(.pf-v6-theme-glass) & block with an
assignment to the masthead background CSS custom property (set
--#{$masthead}--BackgroundColor:
var(--pf-t--global--background--color--glass--primary--default)) so
expandable-content picks up the glass value (remove or comment out the existing
background-color line and ensure border, backdrop-filter, and box-shadow
remain).
In `@src/patternfly/demos/CardView/examples/CardView.md`:
- Around line 24-29: The three page-main-section usages pass the same template
parameter card-template-gallery--id ("card-view-basic-example-gallery") which
causes duplicate DOM ids from the card-template-gallery template; update each
invocation of the card-template-gallery partial to pass a unique id string
(e.g., "card-view-basic-example-gallery-1", "-2", "-3" or descriptive suffixes)
so the generated ids (like card-view-basic-example-gallery-card-1-check) are
unique across sections; modify the three instances where card-template-gallery
is referenced (the page-main-section blocks) to use distinct
card-template-gallery--id values and verify any related aria-labelledby or name
attributes in the template continue to derive from that parameter.
In `@src/patternfly/patternfly.scss`:
- Around line 30-32: The .pf-v6-c-page__sidebar overflow declaration is
duplicated between patternfly.scss (setting overflow: auto) and page.scss
(setting overflow: visible under :where(.pf-v6-theme-glass)), causing a fragile
load-order override; remove the redundant
10BC0
overflow: auto from the
:where(.pf-v6-theme-glass) block in patternfly.scss (or move the desired
overflow rule into page.scss so a single theme-specific override exists) so that
only one authoritative overflow rule for .pf-v6-c-page__sidebar remains and
intent is clear.
---
Nitpick comments:
In `@src/patternfly/components/Drawer/drawer.scss`:
- Around line 862-865: Remove the empty commented placeholder block containing
":where(.pf-v6-theme-glass)" in drawer.scss: delete the three-line commented
section (// :where(.pf-v6-theme-glass) { // } ) so no stray empty comment
remains in the Drawer styles.
In `@src/patternfly/components/Page/page.scss`:
- Around line 316-350: Remove the leftover commented selector "// &:has(>
.#{$page}__sidebar-main)" in the `@at-root` :where(.pf-v6-theme-glass) block of
page.scss to avoid confusion, or if you intend to keep it as a deliberate note,
replace it with a short explanatory comment indicating why the &:has(...)
variant is preserved for future use; reference the surrounding selectors
.pf-v6-theme-glass and .#{$page}__sidebar-main so reviewers can locate the
change.
In `@src/patternfly/demos/Card/card-template-gallery.hbs`:
- Line 15: The demo uses the pf-m-secondary modifier on the selectable card
(card--id built from card-template-gallery--id and
card--modifier="pf-m-selectable-raised pf-m-secondary pf-m-compact") but the CSS
TODO in the card styles leaves secondary cards without a border; decide whether
this is intentional for the glass theme WIP—if not, update the card SCSS rule
that targets .pf-m-secondary (and the selectable variant .pf-m-selectable-raised
.pf-m-secondary) to add the intended border styles (undo the commented TODO
lines around the current card.scss border rules) so the card-1 demo shows full
selectable visual feedback; if intentional, add a brief inline comment near the
markup (card-template-gallery--id / card--modifier usage) stating the missing
border is deliberate to avoid future churn.
In `@src/patternfly/patternfly.scss`:
- Around line 8-17: Remove the POC artifacts in the .pf-v6-theme-glass block:
delete the inline comment "// custom stuff for the POC" and remove the
commented-out localhost background-image line so only the intended production
asset remains; confirm the selectors .pf-v6-c-page, .pf-v6-c-login, and body
keep a single clean background-image rule (or move the image path to a
configuration variable) and ensure no localhost/debug URLs or commented debug
lines remain before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c676fa89-172c-4ce3-9cdb-6ac6908475d0
⛔ Files ignored due to path filters (5)
src/patternfly/assets/images/compass--hero-bg.pngis excluded by!**/*.pngsrc/patternfly/assets/images/compass--wallpaper-dark.jpgis excluded by!**/*.jpgsrc/patternfly/assets/images/compass--wallpaper-light.jpgis excluded by!**/*.jpgsrc/patternfly/assets/images/glass-brand-dark.pngis excluded by!**/*.pngsrc/patternfly/assets/images/glass-brand-light.pngis excluded by!**/*.png
📒 Files selected for processing (27)
src/patternfly/base/tokens/tokens-dark.scsssrc/patternfly/base/tokens/tokens-default.scsssrc/patternfly/base/tokens/tokens-glass-dark.scsssrc/patternfly/base/tokens/tokens-glass.scsssrc/patternfly/base/tokens/tokens-local.scsssrc/patternfly/base/tokens/tokens-palette.scsssrc/patternfly/base/tokens/tokens-redhat-dark.scsssrc/patternfly/base/tokens/tokens-redhat-glass-dark.scsssrc/patternfly/base/tokens/tokens-redhat-glass.scsssrc/patternfly/base/tokens/tokens-redhat-highcontrast-dark.scsssrc/patternfly/base/tokens/tokens-redhat-highcontrast.scsssrc/patternfly/base/tokens/tokens-redhat.scsssrc/patternfly/components/Accordion/accordion.scsssrc/patternfly/components/Card/card.scsssrc/patternfly/components/DataList/data-list.scsssrc/patternfly/components/Drawer/drawer.scsssrc/patternfly/components/ExpandableSection/examples/ExpandableSection.mdsrc/patternfly/components/ExpandableSection/expandable-section.scsssrc/patternfly/components/Login/login.scsssrc/patternfly/components/Masthead/masthead.scsssrc/patternfly/components/Page/page-sidebar.hbssrc/patternfly/components/Page/page.scsssrc/patternfly/components/Table/table-grid.scsssrc/patternfly/components/Table/table.scsssrc/patternfly/demos/Card/card-template-gallery.hbssrc/patternfly/demos/CardView/examples/CardView.mdsrc/patternfly/patternfly.scss
Comment on lines
231
to
235
| &.pf-m-secondary { | ||
| --#{$card}--BorderColor: var(--#{$card}--m-secondary--BorderColor); | ||
| --#{$card}--BorderWidth: var(--#{$card}--m-secondary--BorderWidth); | ||
| // TODO - let selectable secondary cards have a border | ||
| // --#{$card}--BorderColor: var(--#{$card}--m-secondary--BorderColor); | ||
| // --#{$card}--BorderWidth: var(--#{$card}--m-secondary--BorderWidth); | ||
| --#{$card}--BackgroundColor: var(--#{$card}--m-secondary--BackgroundColor); |
Contributor
There was a problem hiding this comment.
This drops the secondary border from every card, not just selectable ones.
The TODO calls out selectable secondary cards, but commenting out these assignments changes the entire .pf-m-secondary variant. Regular secondary cards now fall back to the base border tokens, so they lose the higher-contrast secondary border styling as well.
Suggested fix
&.pf-m-secondary {
- // TODO - let selectable secondary cards have a border
- // --#{$card}--BorderColor: var(--#{$card}--m-secondary--BorderColor);
- // --#{$card}--BorderWidth: var(--#{$card}--m-secondary--BorderWidth);
+ --#{$card}--BorderColor: var(--#{$card}--m-secondary--BorderColor);
+ --#{$card}--BorderWidth: var(--#{$card}--m-secondary--BorderWidth);
--#{$card}--BackgroundColor: var(--#{$card}--m-secondary--BackgroundColor);
}If selectable/clickable secondary cards need different treatment, that should be handled with a narrower override on those combined states instead of on .pf-m-secondary itself.
📝 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.
Suggested change
| &.pf-m-secondary { | |
| --#{$card}--BorderColor: var(--#{$card}--m-secondary--BorderColor); | |
| --#{$card}--BorderWidth: var(--#{$card}--m-secondary--BorderWidth); | |
| // TODO - let selectable secondary cards have a border | |
| // --#{$card}--BorderColor: var(--#{$card}--m-secondary--BorderColor); | |
| // --#{$card}--BorderWidth: var(--#{$card}--m-secondary--BorderWidth); | |
| --#{$card}--BackgroundColor: var(--#{$card}--m-secondary--BackgroundColor); | |
| &.pf-m-secondary { | |
| --#{$card}--BorderColor: var(--#{$card}--m-secondary--BorderColor); | |
| --#{$card}--BorderWidth: var(--#{$card}--m-secondary--BorderWidth); | |
| --#{$card}--BackgroundColor: var(--#{$card}--m-secondary--BackgroundColor); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/components/Card/card.scss` around lines 231 - 235, The
.pf-m-secondary block currently comments out the border token assignments
causing all secondary cards to lose their secondary border; restore the two
assignments (--#{$card}--BorderColor and --#{$card}--BorderWidth) inside the
.pf-m-secondary rule so that non-selectable secondary cards keep their intended
border tokens, and if selectable/clickable secondary cards should differ, add a
narrower override on the combined selector (e.g., the .pf-m-secondary selector
combined with the selectable/clickable modifier such as .pf-m-selectable or
.pf-m-clickable) to change those border tokens only for that combined state
rather than removing them from .pf-m-secondary itself.
Comment on lines
+214
to
+220
| @at-root :where(.pf-v6-theme-glass) & { | ||
| margin-block-end: var(--pf-t--global--spacer--inset--page-chrome); | ||
| background-color: var(--pf-t--global--background--color--glass--primary--default); | ||
| backdrop-filter: var(--pf-t--global--background--filter--glass--blur--primary); | ||
| border-block-end: var(--pf-t--global--border--width--glass--default) solid var(--pf-t--global--border--color--glass--default); | ||
| box-shadow: var(--pf-t--global--box-shadow--glass--default); | ||
| } |
Contributor
There was a problem hiding this comment.
Glass theme override bypasses the masthead background variable.
Line 216 sets the wrapper background directly, but .#{$masthead}__expandable-content still reads --#{$masthead}--BackgroundColor later in this file. In the glass theme, the masthead itself will render with the glass background while expandable toolbar content keeps the default secondary background.
Suggested fix
`@at-root` :where(.pf-v6-theme-glass) & {
+ --#{$masthead}--BackgroundColor: var(--pf-t--global--background--color--glass--primary--default);
margin-block-end: var(--pf-t--global--spacer--inset--page-chrome);
- background-color: var(--pf-t--global--background--color--glass--primary--default);
backdrop-filter: var(--pf-t--global--background--filter--glass--blur--primary);
border-block-end: var(--pf-t--global--border--width--glass--default) solid var(--pf-t--global--border--color--glass--default);
box-shadow: var(--pf-t--global--box-shadow--glass--default);
}📝 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.
Suggested change
| @at-root :where(.pf-v6-theme-glass) & { | |
| margin-block-end: var(--pf-t--global--spacer--inset--page-chrome); | |
| background-color: var(--pf-t--global--background--color--glass--primary--default); | |
| backdrop-filter: var(--pf-t--global--background--filter--glass--blur--primary); | |
| border-block-end: var(--pf-t--global--border--width--glass--default) solid var(--pf-t--global--border--color--glass--default); | |
| box-shadow: var(--pf-t--global--box-shadow--glass--default); | |
| } | |
| `@at-root` :where(.pf-v6-theme-glass) & { | |
| --#{$masthead}--BackgroundColor: var(--pf-t--global--background--color--glass--primary--default); | |
| margin-block-end: var(--pf-t--global--spacer--inset--page-chrome); | |
| backdrop-filter: var(--pf-t--global--background--filter--glass--blur--primary); | |
| border-block-end: var(--pf-t--global--border--width--glass--default) solid var(--pf-t--global--border--color--glass--default); | |
| box-shadow: var(--pf-t--global--box-shadow--glass--default); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/components/Masthead/masthead.scss` around lines 214 - 220, The
glass-theme override is forcing background-color on the masthead, bypassing the
masthead background CSS variable used by .#{$masthead}__expandable-content;
replace the direct background-color assignment inside the `@at-root`
:where(.pf-v6-theme-glass) & block with an assignment to the masthead background
CSS custom property (set --#{$masthead}--BackgroundColor:
var(--pf-t--global--background--color--glass--primary--default)) so
expandable-content picks up the glass value (remove or comment out the existing
background-color line and ensure border, backdrop-filter, and box-shadow
remain).
Comment on lines
+24
to
+29
| {{#> page-main-section page-main-section--modifier="pf-m-secondary"}} | ||
| {{> card-template-gallery card-template-gallery--id="card-view-basic-example-gallery"}} | ||
| {{/page-main-section}} | ||
| {{#> page-main-section}} | ||
| {{> card-template-gallery card-template-gallery--id="card-view-basic-example-gallery"}} | ||
| {{/page-main-section}} |
Contributor
There was a problem hiding this comment.
Critical: Duplicate IDs violate HTML standards and break accessibility.
All three page-main-section blocks (lines 22, 25, 28) use the same card-template-gallery--id="card-view-basic-example-gallery". According to the template in card-template-gallery.hbs, this generates multiple HTML elements with IDs like card-view-basic-example-gallery-card-1-check, card-view-basic-example-gallery-card-2-check, etc. When rendered three times with the same ID parameter, all sections will produce identical IDs.
This causes:
- HTML validation failures (IDs must be unique)
- Broken accessibility (ARIA
aria-labelledbyattributes will reference wrong elements) - Unpredictable behavior for JavaScript selectors and form submissions
- Checkbox inputs with duplicate
nameandidattributes will interfere with each other
🔧 Proposed fix with unique IDs for each section
{{#> page-main-section}}
{{> card-template-gallery card-template-gallery--id="card-view-basic-example-gallery"}}
{{/page-main-section}}
- {{#> page-main-section page-main-section--modifier="pf-m-secondary"}}
- {{> card-template-gallery card-template-gallery--id="card-view-basic-example-gallery"}}
+ {{#> page-main-section page-main-section--modifier="pf-m-secondary"}}
+ {{> card-template-gallery card-template-gallery--id="card-view-secondary-example-gallery"}}
{{/page-main-section}}
- {{#> page-main-section}}
- {{> card-template-gallery card-template-gallery--id="card-view-basic-example-gallery"}}
+ {{#> page-main-section}}
+ {{> card-template-gallery card-template-gallery--id="card-view-tertiary-example-gallery"}}
{{/page-main-section}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/demos/CardView/examples/CardView.md` around lines 24 - 29, The
three page-main-section usages pass the same template parameter
card-template-gallery--id ("card-view-basic-example-gallery") which causes
duplicate DOM ids from the card-template-gallery template; update each
invocation of the card-template-gallery partial to pass a unique id string
(e.g., "card-view-basic-example-gallery-1", "-2", "-3" or descriptive suffixes)
so the generated ids (like card-view-basic-example-gallery-card-1-check) are
unique across sections; modify the three instances where card-template-gallery
is referenced (the page-main-section blocks) to use distinct
card-template-gallery--id values and verify any related aria-labelledby or name
attributes in the template continue to derive from that parameter.
Comment on lines
+30
to
+32
| .pf-v6-c-page__sidebar { | ||
| overflow: auto; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for overflow declarations on page__sidebar under glass theme
rg -n "overflow" src/patternfly/components/Page/page.scss | head -20Repository: patternfly/patternfly
Length of output: 402
🏁 Script executed:
#!/bin/bash
# Check context around line 321 in page.scss (glass theme)
sed -n '310,335p' src/patternfly/components/Page/page.scss | cat -n
# Check lines 30-32 in patternfly.scss
sed -n '25,35p' src/patternfly/patternfly.scss | cat -nRepository: patternfly/patternfly
Length of output: 1204
🏁 Script executed:
#!/bin/bash
# Find the parent selector by looking at lines before the `@at-root` block
sed -n '280,320p' src/patternfly/components/Page/page.scss | cat -nRepository: patternfly/patternfly
Length of output: 1972
🏁 Script executed:
#!/bin/bash
# Find the selector that contains all these sidebar rules
rg -n "\.pf-v6-c-page__sidebar\s*\{" src/patternfly/components/Page/page.scssRepository: patternfly/patternfly
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for page__sidebar in the file to find the selector
rg -n "page__sidebar" src/patternfly/components/Page/page.scss | head -5
# Also check the beginning of the file to understand the structure
sed -n '250,280p' src/patternfly/components/Page/page.scss | cat -nRepository: patternfly/patternfly
Length of output: 1280
🏁 Script executed:
#!/bin/bash
# Check the full context of patternfly.scss around lines 25-40
sed -n '1,40p' src/patternfly/patternfly.scss | cat -nRepository: patternfly/patternfly
Length of output: 1226
🏁 Script executed:
#!/bin/bash
# Verify which file loads first to understand the cascade
rg -n "@use\|@import" src/patternfly/patternfly.scss
# Check if page.scss is part of components
find src/patternfly/components -name "page.scss"Repository: patternfly/patternfly
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Check how components are loaded in patternfly.scss
sed -n '1,10p' src/patternfly/patternfly.scss | cat -n
# Check the components index file
cat src/patternfly/components/_index.scss | head -50Repository: patternfly/patternfly
Length of output: 1910
Remove redundant overflow declaration or consolidate theme-specific overrides.
The .pf-v6-c-page__sidebar in patternfly.scss:30-32 sets overflow: auto within the :where(.pf-v6-theme-glass) context. However, page.scss:321 already sets overflow: visible on the same element under the glass theme via @at-root :where(.pf-v6-theme-glass) &. Both declarations use :where() for equal specificity, so the cascade resolves based on load order—patternfly.scss rules load after page.scss, meaning overflow: auto ultimately wins. This duplicate definition is confusing and fragile. Consolidate the overflow logic in a single location to maintain clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/patternfly.scss` around lines 30 - 32, The
.pf-v6-c-page__sidebar overflow declaration is duplicated between
patternfly.scss (setting overflow: auto) and page.scss (setting overflow:
visible under :where(.pf-v6-theme-glass)), causing a fragile load-order
override; remove the redundant overflow: auto from the
:where(.pf-v6-theme-glass) block in patternfly.scss (or move the desired
overflow rule into page.scss so a single theme-specific override exists) so that
only one authoritative overflow rule for .pf-v6-c-page__sidebar remains and
intent is clear.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/patternfly/patternfly.scss (1)
8-9: Clean up POC comment before merge.The comment "custom stuff for the POC" should be removed or replaced with proper documentation before this is merged to main. Consider documenting the purpose of the glass theme background image setup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patternfly/patternfly.scss` around lines 8 - 9, Remove the throwaway POC comment inside the :where(.pf-v6-theme-glass) block and replace it with a concise, descriptive comment describing the intent and any specifics of the glass theme setup (e.g., background-image usage, fallbacks, and purpose for theming), or delete it entirely if no comment is needed; update the comment text to clearly document why the glass theme exists and how it should be maintained for future readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patternfly/components/Accordion/examples/Accordion.md`:
- Line 432: Update the table cell text for the `.pf-m-plain` row so the
description reads correctly; replace "Modifies the accordion have plain styles."
with "Modifies the accordion to have plain styles." in the Accordion example
where the `.pf-m-plain` and `.pf-v6-c-accordion` classes are referenced.
In `@src/patternfly/patternfly.scss`:
- Around line 19-24: Add a blank line before the inline comment that precedes
the commented-out background-image inside the &:where(.pf-v6-theme-dark) block
(the .pf-v6-c-page, .pf-v6-c-login, body selector) so the comment "//
background-image: url("./assets/images/compass--wallpaper-dark.jpg");" is
separated by an empty line to satisfy the linter.
- Around line 10-17: In src/patternfly/patternfly.scss fix the SCSS lint rule by
inserting a single empty line immediately before the inline double-slash comment
that comments out the alternate background-image (the commented line referencing
"./assets/images/compass--wallpaper-light.jpg") inside the .pf-v6-c-page,
.pf-v6-c-login, body block; alternatively remove that commented-out
background-image or replace it with a short TODO comment explaining why it's
kept to avoid the scss/double-slash-comment-empty-line-before violation.
---
Nitpick comments:
In `@src/patternfly/patternfly.scss`:
- Around line 8-9: Remove the throwaway POC comment inside the
:where(.pf-v6-theme-glass) block and replace it with a concise, descriptive
comment describing the intent and any specifics of the glass theme setup (e.g.,
background-image usage, fallbacks, and purpose for theming), or delete it
entirely if no comment is needed; update the comment text to clearly document
why the glass theme exists and how it should be maintained for future readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a206544b-bc9a-47a5-8e38-d7fc3f18c95e
⛔ Files ignored due to path filters (2)
src/patternfly/assets/images/glass-brand-dark.jpgis excluded by!**/*.jpgsrc/patternfly/assets/images/glass-brand-light.jpgis excluded by!**/*.jpg
📒 Files selected for processing (2)
src/patternfly/components/Accordion/examples/Accordion.mdsrc/patternfly/patternfly.scss
| | `.pf-v6-c-accordion__toggle-icon` | `<span>` | Initiates the toggle icon wrapper. **Required** | | ||
| | `.pf-v6-c-accordion__expandable-content` | `<div>`, `<dd>` | Initiates expandable content. **Must be paired with a button** | | ||
| | `.pf-v6-c-accordion__expandable-content-body` | `<div>` | Initiates expandable content body. **Required** | | ||
| | `.pf-m-plain` | `.pf-v6-c-accordion` | Modifies the accordion have plain styles. | |
Contributor
There was a problem hiding this comment.
Fix the usage description grammar.
The new row reads awkwardly as written: Modifies the accordion have plain styles. Please change it to Modifies the accordion to have plain styles.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/components/Accordion/examples/Accordion.md` at line 432,
Update the table cell text for the `.pf-m-plain` row so the description reads
correctly; replace "Modifies the accordion have plain styles." with "Modifies
the accordion to have plain styles." in the Accordion example where the
`.pf-m-plain` and `.pf-v6-c-accordion` classes are referenced.
src/patternfly/patternfly.scss
Outdated
Comment on lines
+10
to
+17
| .pf-v6-c-page, .pf-v6-c-login, body { | ||
| background-image: url("./assets/images/glass-brand-light.jpg"); | ||
| // background-image: url("./assets/images/compass--wallpaper-light.jpg"); | ||
| background-repeat: no-repeat; | ||
| background-attachment: fixed; | ||
| background-position: center; | ||
| background-size: cover; | ||
| } |
Contributor
There was a problem hiding this comment.
Fix lint error: add empty line before comment to resolve pipeline failure.
The pipeline is failing due to the scss/double-slash-comment-empty-line-before rule violation. Add an empty line before the commented-out background-image on line 12.
🔧 Proposed fix
.pf-v6-c-page, .pf-v6-c-login, body {
background-image: url("./assets/images/glass-brand-light.jpg");
+
// background-image: url("./assets/images/compass--wallpaper-light.jpg");
background-repeat: no-repeat;Also, consider removing the commented-out alternative before merging, or add a TODO explaining why it's kept.
📝 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.
Suggested change
| .pf-v6-c-page, .pf-v6-c-login, body { | |
| background-image: url("./assets/images/glass-brand-light.jpg"); | |
| // background-image: url("./assets/images/compass--wallpaper-light.jpg"); | |
| background-repeat: no-repeat; | |
| background-attachment: fixed; | |
| background-position: center; | |
| background-size: cover; | |
| } | .pf-v6-c-page, .pf-v6-c-login, body { |
| background-image: url("./assets/images/glass-brand-light.jpg"); | |
| // background-image: url("./assets/images/compass--wallpaper-light.jpg"); | |
| background-repeat: no-repeat; | |
| background-attachment: fixed; | |
| background-position: center; | |
| background-size: cover; | |
| } |
🧰 Tools
🪛 GitHub Actions: pr-preview
[error] 12-12: scss/double-slash-comment-empty-line-before: Expected empty line before comment.
🪛 Stylelint (17.4.0)
[error] 12-12: Expected empty line before comment (scss/double-slash-comment-empty-line-before)
(scss/double-slash-comment-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/patternfly.scss` around lines 10 - 17, In
src/patternfly/patternfly.scss fix the SCSS lint rule by inserting a single
empty line immediately before the inline double-slash comment that comments out
the alternate background-image (the commented line referencing
"./assets/images/compass--wallpaper-light.jpg") inside the .pf-v6-c-page,
.pf-v6-c-login, body block; alternatively remove that commented-out
background-image or replace it with a short TODO comment explaining why it's
kept to avoid the scss/double-slash-comment-empty-line-before violation.
Comment on lines
+19
to
+24
| &:where(.pf-v6-theme-dark) { | ||
| .pf-v6-c-page, .pf-v6-c-login, body { | ||
| background-image: url("./assets/images/glass-brand-dark.jpg"); | ||
| // background-image: url("./assets/images/compass--wallpaper-dark.jpg"); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Fix lint error: add empty line before comment.
Same lint violation as above on line 22.
🔧 Proposed fix
&:where(.pf-v6-theme-dark) {
.pf-v6-c-page, .pf-v6-c-login, body {
background-image: url("./assets/images/glass-brand-dark.jpg");
+
// background-image: url("./assets/images/compass--wallpaper-dark.jpg");
}
}📝 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.
Suggested change
| &:where(.pf-v6-theme-dark) { | |
| .pf-v6-c-page, .pf-v6-c-login, body { | |
| background-image: url("./assets/images/glass-brand-dark.jpg"); | |
| // background-image: url("./assets/images/compass--wallpaper-dark.jpg"); | |
| } | |
| } | |
| &:where(.pf-v6-theme-dark) { | |
| .pf-v6-c-page, .pf-v6-c-login, body { | |
| background-image: url("./assets/images/glass-brand-dark.jpg"); | |
| // background-image: url("./assets/images/compass--wallpaper-dark.jpg"); | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: pr-preview
[error] 22-22: scss/double-slash-comment-empty-line-before: Expected empty line before comment.
🪛 Stylelint (17.4.0)
[error] 22-22: Expected empty line before comment (scss/double-slash-comment-empty-line-before)
(scss/double-slash-comment-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/patternfly.scss` around lines 19 - 24, Add a blank line before
the inline comment that precedes the commented-out background-image inside the
&:where(.pf-v6-theme-dark) block (the .pf-v6-c-page, .pf-v6-c-login, body
selector) so the comment "// background-image:
url("./assets/images/compass--wallpaper-dark.jpg");" is separated by an empty
line to satisfy the linter.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patternfly/components/Page/page.scss`:
- Around line 179-184: The selector block for the glass theme is missing an
`@at-root`, causing the compiled selector to be nested incorrectly; update the
block that currently starts with ":where(.pf-v6-theme-glass) &" so it is emitted
at root by wrapping it with "@at-root :where(.pf-v6-theme-glass) & { ... }"
(matching the pattern used at line 317) so the CSS becomes
":where(.pf-v6-theme-glass) .pf-c-page" and the variables like
--#{$page}__sidebar--Width--base and
--#{$page}__sidebar-body--PaddingInlineStart/End apply correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0f065733-d256-4070-a0be-6e6c451a3e76
📒 Files selected for processing (1)
src/patternfly/components/Page/page.scss
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/patternfly/components/Page/page.scss (1)
181-185:⚠️ Potential issue | 🔴 CriticalAdd
@at-roothere or the glass overrides still won’t match.This is still nested under
@include pf-root($page), so it compiles as a descendant selector instead of:where(.pf-v6-theme-glass) .pf-v6-c-page. Since the glass class is applied at the root level, these sidebar width/padding variables never take effect.🐛 Proposed fix
- :where(.pf-v6-theme-glass) & { + `@at-root` :where(.pf-v6-theme-glass) & { --#{$page}__sidebar--Width--base: calc(#{pf-size-prem(290px)} + var(--pf-t--global--spacer--inset--page-chrome) * 2); --#{$page}__sidebar-body--PaddingInlineStart: var(--pf-t--global--spacer--lg); --#{$page}__sidebar-body--PaddingInlineEnd: var(--pf-t--global--spacer--lg); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patternfly/components/Page/page.scss` around lines 181 - 185, The glass theme selector is still nested inside `@include` pf-root($page) so ":where(.pf-v6-theme-glass) &" compiles as a descendant instead of targeting the root; move the block for the CSS custom properties (the :where(.pf-v6-theme-glass) & rule that sets --#{$page}__sidebar--Width--base, --#{$page}__sidebar-body--PaddingInlineStart, and --#{$page}__sidebar-body--PaddingInlineEnd) to emit at root by prepending `@at-root` so the overrides apply to .pf-v6-c-page when .pf-v6-theme-glass is on the root element.
🤖 Prompt for all review comments with AI agents
Verify each finding against the curr
8E6B
ent code and only fix it if needed.
Inline comments:
In `@src/patternfly/components/Page/page.scss`:
- Around line 351-353: The global selector using &:has(>
.#{$page}__sidebar-main) { overflow: revert; } is undoing the earlier
.#{$page}__sidebar overflow-x: hidden / overflow-y: auto and must be scoped to
glass only; move or re-add this overflow: revert rule inside the existing
`@at-root` :where(.pf-v6-theme-glass) & block (where .#{$page}__sidebar-main {
overflow: auto; } already lives) so non-glass sidebars keep their original
scroll container behavior and only glass theme sidebars get the overflow reset.
---
Duplicate comments:
In `@src/patternfly/components/Page/page.scss`:
- Around line 181-185: The glass theme selector is still nested inside `@include`
pf-root($page) so ":where(.pf-v6-theme-glass) &" compiles as a descendant
instead of targeting the root; move the block for the CSS custom properties (the
:where(.pf-v6-theme-glass) & rule that sets --#{$page}__sidebar--Width--base,
--#{$page}__sidebar-body--PaddingInlineStart, and
--#{$page}__sidebar-body--PaddingInlineEnd) to emit at root by prepending
`@at-root` so the overrides apply to .pf-v6-c-page when .pf-v6-theme-glass is on
the root element.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e7dfaf59-6ba6-42cc-83a8-3909e4f2c289
📒 Files selected for processing (3)
src/patternfly/components/Login/login.scsssrc/patternfly/components/Page/page.scsssrc/patternfly/demos/Compass/compass--demo-context.hbs
Comment on lines
+351
to
+353
| &:has(> .#{$page}__sidebar-main) { | ||
| overflow: revert; | ||
| } |
Contributor
There was a problem hiding this comment.
Scope this overflow reset to glass, or default-theme sidebars stop scrolling.
These lines override the overflow-x: hidden / overflow-y: auto set earlier on .#{$page}__sidebar, but the replacement scroll container (.#{$page}__sidebar-main { overflow: auto; }) is only configured inside the glass-only block above. With the new wrapper present, non-glass sidebars can spill past the viewport instead of getting their own scrollbar.
🐛 Proposed fix
- &:has(> .#{$page}__sidebar-main) {
- overflow: revert;
- }If this reset is only needed for glass, keep it inside the existing @at-root :where(.pf-v6-theme-glass) & block instead of applying it globally.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/components/Page/page.scss` around lines 351 - 353, The global
selector using &:has(> .#{$page}__sidebar-main) { overflow: revert; } is undoing
the earlier .#{$page}__sidebar overflow-x: hidden / overflow-y: auto and must be
scoped to glass only; move or re-add this overflow: revert rule inside the
existing `@at-root` :where(.pf-v6-theme-glass) & block (where
.#{$page}__sidebar-main { overflow: auto; } already lives) so non-glass sidebars
keep their original scroll container behavior and only glass theme sidebars get
the overflow reset.
b2f41fb to
6862dd9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WIP
Summary by CodeRabbit
New Features
Style
Documentation