feat(Hero): make glass styles glass-theme only - #8365
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughHero and Card SCSS were changed to move hero defaults off glass tokens, update paddings and radii, introduce component-scoped glass CSS custom properties, and re-scope glass styles under the glass theme + ChangesHero & Card theme / glass scoping
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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-8365.surge.sh A11y report: https://pf-pr-8365-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/patternfly/components/Hero/hero.scss`:
- Around line 69-76: The change made glass styling conditional on an explicit
.pf-m-glass modifier inside the theme selector, which breaks existing Heroes in
.pf-v6-theme-glass; revert to applying the glass variables and properties by
default under :where(.#{$pf-prefix}theme-glass) & and instead add an opt-out
modifier .pf-m-no-glass that resets/overrides --#{$hero}--BorderColor,
--#{$hero}--BackgroundColor, --#{$hero}--m-glass--BackdropFilter, and
--#{$hero}--m-glass--BoxShadow (and the backdrop-filter/box-shadow declarations)
so consumers can opt out; keep .pf-m-glass only if you want it to explicitly
re-enable glass in non-theme contexts and ensure hero.hbs continues to only
output modifiers (no implicit behavior change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 83040592-02cd-474b-961b-65ec91b26d23
📒 Files selected for processing (2)
src/patternfly/components/Hero/examples/Hero.mdsrc/patternfly/components/Hero/hero.scss
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/patternfly/components/Card/card.scss (1)
143-150: 💤 Low valueUse
$pf-prefixinterpolation for the theme-glass selector to stay consistent with Hero.
hero.scssL69 in this same PR writes the parallel selector as:where(.#{$pf-prefix}theme-glass) &.pf-m-glass, while here the prefix is hardcoded aspf-v6-. Both produce the same class today, but the hardcoded form is easy to miss on a future prefix bump and diverges from the pattern used elsewhere in the codebase.♻️ Proposed change
- :where(.pf-v6-theme-glass) &.pf-m-glass { + :where(.#{$pf-prefix}theme-glass) &.pf-m-glass { --#{$card}--BackgroundColor: var(--#{$card}--m-glass--BackgroundColor); --#{$card}--BorderColor: var(--#{$card}--m-glass--BorderColor); --#{$card}--BorderWidth: var(--#{$card}--m-glass--BorderWidth); - + backdrop-filter: var(--#{$card}--m-glass--BackdropFilter); box-shadow: var(--#{$card}--m-glass--BoxShadow); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/patternfly/components/Card/card.scss` around lines 143 - 150, The theme selector in Card (the :where(.pf-v6-theme-glass) &.pf-m-glass block) hardcodes the prefix; change it to use the $pf-prefix interpolation (i.e., :where(.#{$pf-prefix}theme-glass) &.pf-m-glass) so it matches hero.scss and will update correctly on prefix changes—update the selector only, leaving the inner CSS custom properties (e.g., --#{$card}--BackgroundColor, backdrop-filter, box-shadow) unchanged.src/patternfly/components/Hero/hero.scss (1)
82-82: 💤 Low valueTrailing whitespace after closing brace.
Minor cleanup — there is a stray space after
}on this line.🧹 Proposed cleanup
- } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/patternfly/components/Hero/hero.scss` at line 82, There is a stray trailing space after a closing brace in the Hero stylesheet; remove the extra whitespace character that follows the `}` in hero.scss so the closing brace is the final character on that line (no code changes beyond trimming the trailing space).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/patternfly/components/Card/card.scss`:
- Around line 143-150: The theme selector in Card (the
:where(.pf-v6-theme-glass) &.pf-m-glass block) hardcodes the prefix; change it
to use the $pf-prefix interpolation (i.e., :where(.#{$pf-prefix}theme-glass)
&.pf-m-glass) so it matches hero.scss and will update correctly on prefix
changes—update the selector only, leaving the inner CSS custom properties (e.g.,
--#{$card}--BackgroundColor, backdrop-filter, box-shadow) unchanged.
In `@src/patternfly/components/Hero/hero.scss`:
- Line 82: There is a stray trailing space after a closing brace in the Hero
stylesheet; remove the extra whitespace character that follows the `}` in
hero.scss so the closing brace is the final character on that line (no code
changes beyond trimming the trailing space).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 84330ac9-fe8c-4c06-bd75-b912cb59d37b
📒 Files selected for processing (2)
src/patternfly/components/Card/card.scsssrc/patternfly/components/Hero/hero.scss
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/patternfly/demos/Compass/examples/Compass.md`:
- Line 33: The hero component invocation is passing the boolean modifier as a
bare positional argument (hero--IsGlass) which doesn't set the modifier; update
the hero invocation to pass the hash boolean explicitly (hero--IsGlass=true) so
the setModifiers logic applies and the pf-m-glass CSS class is added; locate the
hero usage ({{#> hero hero--IsGlass}}) and change the argument to the hash-style
boolean (hero--IsGlass=true) consistent with other modifiers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2892998d-ffdc-4f1a-a048-78acb5da48c9
📒 Files selected for processing (2)
src/patternfly/components/Hero/hero.hbssrc/patternfly/demos/Compass/examples/Compass.md
|
🎉 This PR is included in version 6.5.0-prerelease.82 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
Closes #8316.
Moves gradient, backdrop filter, and background image styles to glass theme only block (unlesspf-m-no-glass).Adds background color to base styles & set to primary background color (glass theme resets color to glass token).
Do we want to add a plain modifier opt in for non-glass theme? Hero is pretty opinionated in its styling so unsure if it's necessary to proactively add one.Updated:
pf-m-glass, making Hero glass styles opt-inpf-m-no-glassSummary by CodeRabbit
Style
Refactor
Documentation