feat(card): update card to use unified theme tokens#8180
feat(card): update card to use unified theme tokens#8180wise-king-sullyman wants to merge 35 commits into
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:
WalkthroughReplaces inline Font Awesome/pficon Changes
Sequence Diagram(s)(Skipped — changes are template/style updates without new multi-component runtime flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
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. 📝 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 |
|
Preview: https://pf-pr-8180.surge.sh A11y report: https://pf-pr-8180-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: 4
🤖 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`:
- Line 9: The global card box-shadow variable (--#{$card}--BoxShadow) is being
applied to all cards causing .pf-m-plain cards to keep elevation; update the
stylesheet so plain cards do not inherit it by either changing the rule that
applies the variable (or the box-shadow usage) to exclude elements with the
.pf-m-plain modifier, or explicitly reset the variable for the plain modifier
(e.g. add a .pf-m-plain selector that sets --#{$card}--BoxShadow: none or 0) so
.pf-m-plain cards have no elevation.
- Around line 209-212: The current selector "&.pf-m-selectable" is too broad and
applies the tile variables to all selectable cards; narrow it so the tile
background/border tokens only apply when the card is also in the tile mode by
changing the selector to require both classes (e.g., use a combined selector
such as &:is(.pf-m-selectable.pf-m-as-tile) or &.pf-m-selectable.pf-m-as-tile)
where the variables --#{$card}--m-as-tile--BackgroundColor and
--#{$card}--m-as-tile--BorderColor are set, ensuring only tile cards receive
those tokens.
In `@src/patternfly/demos/Card/examples/Card.md`:
- Around line 582-586: The icon used for the "Ready" status is semantically
incorrect: inside the template block with icon-content--modifier="pf-m-success"
the helper pfIcon is rendering "rh-ui-error"; update the template in Card.md so
the pfIcon invocation uses a success icon (replace "rh-ui-error" with
"rh-ui-check-circle") so the icon matches the pf-m-success modifier.
- Around line 633-637: The icon markup uses the error icon name "rh-ui-error"
while applying the success modifier "pf-m-success" (in the icon-content and
pfIcon usage), causing a semantic mismatch; update the pfIcon call to use the
success icon (e.g., "rh-ui-success") when rendering the Ready status so the icon
name matches the pf-m-success modifier (or, if the intent is to show an error,
change the modifier to pf-m-danger instead) — look for the template block with
the icon partials (icon, icon-content) and the pfIcon "rh-ui-error" and make the
appropriate replacement.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
src/patternfly/components/Card/card-header-toggle-icon.hbssrc/patternfly/components/Card/card.scsssrc/patternfly/demos/Card/card-template-gallery.hbssrc/patternfly/demos/Card/examples/Card.mdsrc/patternfly/demos/Card/templates/card-demo--popover-table.hbssrc/patternfly/demos/Card/templates/card-demo--status-card.hbssrc/patternfly/demos/Card/templates/card-demo--template-gallery.hbssrc/patternfly/demos/Card/templates/card-template-data-list.hbssrc/patternfly/demos/Card/templates/card-template-events.hbssrc/patternfly/demos/Card/templates/card-template-expandable-status-card.hbssrc/patternfly/demos/Card/templates/card-template-expandable-status.hbssrc/patternfly/demos/Card/templates/card-template-status.hbs
| {{#> icon-content icon-content--modifier="pf-m-success"}} | ||
| <i class="fas fa-exclamation-circle" aria-hidden="true"></i> | ||
| {{pfIcon "rh-ui-error"}} | ||
| {{/icon-content}} | ||
| {{/icon}} | ||
| {{/l-flex-item}} |
There was a problem hiding this comment.
Semantic mismatch: error icon used with success modifier.
The icon rh-ui-error is used inside icon-content--modifier="pf-m-success" for a "Ready" status. This appears to be a copy-paste error. A "Ready" state should use a success-themed icon like rh-ui-check-circle.
🐛 Proposed fix
{{#> icon icon--modifier="pf-m-inline"}}
{{#> icon-content icon-content--modifier="pf-m-success"}}
- {{pfIcon "rh-ui-error"}}
+ {{pfIcon "rh-ui-check-circle"}}
{{/icon-content}}
{{/icon}}📝 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.
| {{#> icon-content icon-content--modifier="pf-m-success"}} | |
| <i class="fas fa-exclamation-circle" aria-hidden="true"></i> | |
| {{pfIcon "rh-ui-error"}} | |
| {{/icon-content}} | |
| {{/icon}} | |
| {{/l-flex-item}} | |
| {{#> icon-content icon-content--modifier="pf-m-success"}} | |
| {{pfIcon "rh-ui-check-circle"}} | |
| {{/icon-content}} | |
| {{/icon}} | |
| {{/l-flex-item}} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/demos/Card/examples/Card.md` around lines 582 - 586, The icon
used for the "Ready" status is semantically incorrect: inside the template block
with icon-content--modifier="pf-m-success" the helper pfIcon is rendering
"rh-ui-error"; update the template in Card.md so the pfIcon invocation uses a
success icon (replace "rh-ui-error" with "rh-ui-check-circle") so the icon
matches the pf-m-success modifier.
There was a problem hiding this comment.
This is funky, but it matches what we had before.
There was a problem hiding this comment.
@wise-king-sullyman, understood! If this preserves the existing behavior from before the icon migration, then maintaining consistency makes sense for this PR. Thanks for clarifying.
| {{#> icon icon--modifier="pf-m-inline"}} | ||
| {{#> icon-content icon-content--modifier="pf-m-success"}} | ||
| <i class="fas fa-exclamation-circle" aria-hidden="true"></i> | ||
| {{pfIcon "rh-ui-error"}} | ||
| {{/icon-content}} | ||
| {{/icon}} |
There was a problem hiding this comment.
Same semantic mismatch: error icon with success modifier.
Same issue as above - using rh-ui-error with pf-m-success for "Ready" status.
🐛 Proposed fix
{{#> icon icon--modifier="pf-m-inline"}}
{{#> icon-content icon-content--modifier="pf-m-success"}}
- {{pfIcon "rh-ui-error"}}
+ {{pfIcon "rh-ui-check-circle"}}
{{/icon-content}}
{{/icon}}📝 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.
| {{#> icon icon--modifier="pf-m-inline"}} | |
| {{#> icon-content icon-content--modifier="pf-m-success"}} | |
| <i class="fas fa-exclamation-circle" aria-hidden="true"></i> | |
| {{pfIcon "rh-ui-error"}} | |
| {{/icon-content}} | |
| {{/icon}} | |
| {{#> icon icon--modifier="pf-m-inline"}} | |
| {{#> icon-content icon-content--modifier="pf-m-success"}} | |
| {{pfIcon "rh-ui-check-circle"}} | |
| {{/icon-content}} | |
| {{/icon}} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/demos/Card/examples/Card.md` around lines 633 - 637, The icon
markup uses the error icon name "rh-ui-error" while applying the success
modifier "pf-m-success" (in the icon-content and pfIcon usage), causing a
semantic mismatch; update the pfIcon call to use the success icon (e.g.,
"rh-ui-success") when rendering the Ready status so the icon name matches the
pf-m-success modifier (or, if the intent is to show an error, change the
modifier to pf-m-danger instead) — look for the template block with the icon
partials (icon, icon-content) and the pfIcon "rh-ui-error" and make the
appropriate replacement.
There was a problem hiding this comment.
This is funky, but it matches what we had before.
There was a problem hiding this comment.
@wise-king-sullyman, understood! Since you're preserving the existing behavior from the previous implementation, that makes sense for this PR's scope. I won't push for this change. Thanks for the clarification!
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/patternfly/components/Card/card.scss (2)
108-110:⚠️ Potential issue | 🟠 MajorPlain cards still inherit elevation from the new global shadow.
Line 129 applies
--#{$card}--BoxShadowglobally, but.pf-m-plain(Line 256-259) does not reset it, so plain cards can still render with elevation.Proposed fix
// Plain - no border or background color --#{$card}--m-plain--BorderColor: transparent; --#{$card}--m-plain--BackgroundColor: transparent; + --#{$card}--m-plain--BoxShadow: none;&.pf-m-plain { --#{$card}--BorderColor: var(--#{$card}--m-plain--BorderColor); --#{$card}--BackgroundColor: var(--#{$card}--m-plain--BackgroundColor); + --#{$card}--BoxShadow: var(--#{$card}--m-plain--BoxShadow); }Also applies to: 129-129, 256-259
🤖 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 108 - 110, The plain card variant is not resetting the global box-shadow variable, so update the .pf-m-plain rule to reset the shadow by setting --#{$card}--BoxShadow: none (or 0 0 transparent) and ensure any applied box-shadow on .pf-c-card uses that variable; modify the .pf-m-plain selector in card.scss to explicitly clear the shadow using the --#{$card}--BoxShadow variable so plain cards do not inherit elevation from the global BoxShadow declaration.
209-213:⚠️ Potential issue | 🟠 MajorAs-tile styling is still scoped too broadly to all selectable cards.
Line 210 (
&.pf-m-selectable) applies tile background/border tokens to non-tile selectable cards too.Proposed fix
- `@at-root` .#{$card}__actions:has(> .#{$card}__selectable-actions input.#{$pf-prefix}screen-reader), // targets "card as tile", which are selectable cards with inputs - &.pf-m-selectable { + // Target only card-as-tile (selectable cards with screen-reader input) + &:has(.#{$card}__selectable-actions input.#{$pf-prefix}screen-reader) { --#{$card}--BackgroundColor: var(--#{$card}--m-as-tile--BackgroundColor); --#{$card}--BorderColor: var(--#{$card}--m-as-tile--BorderColor); }🤖 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 209 - 213, The selector currently applies as-tile tokens to all selectable cards because it uses &.pf-m-selectable; narrow the selector to only target selectable cards that are also "as-tile" by combining the modifiers—for example replace &.pf-m-selectable with &.pf-m-selectable.pf-m-as-tile (or the equivalent class interpolation for the as-tile modifier) so the block that sets --#{$card}--BackgroundColor and --#{$card}--BorderColor from --#{$card}--m-as-tile--BackgroundColor / --#{$card}--m-as-tile--BorderColor only runs for cards that are both pf-m-selectable and pf-m-as-tile (referencing .#{$card}__actions, .#{$card}__selectable-actions and the pf-prefix screen-reader selector in the same rule).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/patternfly/components/Card/card.scss`:
- Around line 108-110: The plain card variant is not resetting the global
box-shadow variable, so update the .pf-m-plain rule to reset the shadow by
setting --#{$card}--BoxShadow: none (or 0 0 transparent) and ensure any applied
box-shadow on .pf-c-card uses that variable; modify the .pf-m-plain selector in
card.scss to explicitly clear the shadow using the --#{$card}--BoxShadow
variable so plain cards do not inherit elevation from the global BoxShadow
declaration.
- Around line 209-213: The selector currently applies as-tile tokens to all
selectable cards because it uses &.pf-m-selectable; narrow the selector to only
target selectable cards that are also "as-tile" by combining the modifiers—for
example replace &.pf-m-selectable with &.pf-m-selectable.pf-m-as-tile (or the
equivalent class interpolation for the as-tile modifier) so the block that sets
--#{$card}--BackgroundColor and --#{$card}--BorderColor from
--#{$card}--m-as-tile--BackgroundColor / --#{$card}--m-as-tile--BorderColor only
runs for cards that are both pf-m-selectable and pf-m-as-tile (referencing
.#{$card}__actions, .#{$card}__selectable-actions and the pf-prefix
screen-reader selector in the same rule).
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/Card/card.scss`:
- Around line 209-214: Update the stale comment above the tile-mode selector to
describe what the rule actually does: when the whole card is clickable and the
card actions are invisible (selectors using .#{$card}__actions and
.#{$card}__selectable-actions with input.#{$pf-prefix}screen-reader), the rule
applies tile-mode visual variables by setting --#{$card}--BackgroundColor and
--#{$card}--BorderColor to their m-as-tile variants; replace the old "remove the
padding/margin" wording with a concise note that this block switches the card to
tile visual styles (background and border) for clickable/selectable cards.
| // when the whole card is clickable and the card actions are invisible, remove the padding/margin so it doesn't take up space | ||
| @at-root .#{$card}__actions:has(> .#{$card}__selectable-actions input.#{$pf-prefix}screen-reader), // targets "card as tile", which are selectable cards with inputs | ||
| &:has(.#{$card}__selectable-actions input.#{$pf-prefix}screen-reader) { | ||
| --#{$card}--BackgroundColor: var(--#{$card}--m-as-tile--BackgroundColor); | ||
| --#{$card}--BorderColor: var(--#{$card}--m-as-tile--BorderColor); | ||
| } |
There was a problem hiding this comment.
Update the stale inline comment above the tile-mode selector.
The comment says this block removes actions padding/margin, but the block now sets tile background/border variables. Please align the comment with current behavior to avoid future misreads.
🤖 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 209 - 214, Update the
stale comment above the tile-mode selector to describe what the rule actually
does: when the whole card is clickable and the card actions are invisible
(selectors using .#{$card}__actions and .#{$card}__selectable-actions with
input.#{$pf-prefix}screen-reader), the rule applies tile-mode visual variables
by setting --#{$card}--BackgroundColor and --#{$card}--BorderColor to their
m-as-tile variants; replace the old "remove the padding/margin" wording with a
concise note that this block switches the card to tile visual styles (background
and border) for clickable/selectable cards.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/patternfly/components/Card/card.scss (1)
209-214:⚠️ Potential issue | 🟡 MinorInline comment is stale for the selector block behavior.
Line [209]still mentions removing padding/margin, but this block now sets tile background/border variables (Lines [212-213]).🤖 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 209 - 214, The existing inline comment incorrectly says this selector "removes padding/margin" but the selector block (.#{$card}__actions:has(> .#{$card}__selectable-actions input.#{$pf-prefix}screen-reader and &:has(.#{$card}__selectable-actions input.#{$pf-prefix}screen-reader)) actually sets tile-specific CSS variables (--#{$card}--BackgroundColor and --#{$card}--BorderColor); update the comment to accurately describe that this rule applies to cards rendered "as tile" with selectable actions/screen-reader inputs and overrides the card background and border variables to the m-as-tile values (remove the padding/margin claim and mention the variable overrides and intent).
🤖 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 251-254: The .pf-m-secondary .pf-m-selectable/.pf-m-clickable rule
is overriding the .pf-m-current and .pf-m-selected border styles; update the CSS
so secondary selectable/clickable cards do not clobber selected/current borders
by adding more specific rules that restore the selected/current variables for
secondary cards (e.g. add selectors like
.pf-m-secondary.pf-m-selectable.pf-m-current,
.pf-m-secondary.pf-m-selectable.pf-m-selected, and the same for .pf-m-clickable
to set --#{$card}--BorderColor and --#{$card}--BorderWidth back to the
selected/current variable values), or alternatively scope the original rule to
exclude .pf-m-current and .pf-m-selected (using
:not(.pf-m-current):not(.pf-m-selected)) so the secondary selectable/clickable
variables only apply when the card is not selected/current.
---
Duplicate comments:
In `@src/patternfly/components/Card/card.scss`:
- Around line 209-214: The existing inline comment incorrectly says this
selector "removes padding/margin" but the selector block
(.#{$card}__actions:has(> .#{$card}__selectable-actions
input.#{$pf-prefix}screen-reader and &:has(.#{$card}__selectable-actions
input.#{$pf-prefix}screen-reader)) actually sets tile-specific CSS variables
(--#{$card}--BackgroundColor and --#{$card}--BorderColor); update the comment to
accurately describe that this rule applies to cards rendered "as tile" with
selectable actions/screen-reader inputs and overrides the card background and
border variables to the m-as-tile values (remove the padding/margin claim and
mention the variable overrides and intent).
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/patternfly/components/Card/card-header-toggle-icon.hbssrc/patternfly/components/Card/card.scss
| --#{$card}__header-toggle-icon--TransitionTimingFunction: var(--pf-t--global--motion--timing-function--default); | ||
| --#{$card}__header-toggle-icon--TransitionDuration: var(--pf-t--global--motion--duration--icon--default); | ||
| --#{$card}--m-expanded__header-toggle-icon--Rotate: 90deg; | ||
| --#{$card}--m-expanded__header-toggle-icon--Rotate: -180deg; |
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/Card/card.scss`:
- Around line 252-256: The selector uses chained :not() pseudo-classes which
breaks the stylelint selector-not-notation rule; update the two selectors (the
ones starting with &.pf-m-selectable and &.pf-m-clickable) to use a single
complex :not() with a comma-separated list like :not(.pf-m-current,
.pf-m-selected) and keep the existing CSS custom property assignments (e.g.,
--#{$card}--BorderColor and --#{$card}--BorderWidth) unchanged.
|
hey @wise-king-sullyman! this all is looking really good -- wondering if we can go ahead and add a |
lboehling
left a comment
lboehling
left a comment
There was a problem hiding this comment.
lgtm! thanks austin!
I'll note that the basic static cards kinda blend into the background in dark/default mode but i am going to tweak the box shadow and subtle border color tokens slightly so it appears more contrasted in non-glass modes -- no additional work needed for those updates in the card component itself since they'll come with the global token values!
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
Left a few commens. One other thing I noticed is that we're using both filled and outlined status icons, and I wasn't sure if that was on purpose or not?
Here's the backstop report for reference
| --#{$card}--m-as-tile--BackgroundColor: var(--pf-t--global--background--color--control--default); | ||
| --#{$card}--m-as-tile--BorderColor: var(--pf-t--global--border--color--control--default); |
There was a problem hiding this comment.
Also I saw we have a follow up design issue to add a tile variant of card. If so, and you think we'll have a .pf-m-tile modifier class for tiles, I would rename these vars to --m-tile instead of --m-as-tile since that's how we usually name variants. If you don't anticipate us adding the modifier class, you can always just use --tile in the var name. I would love to have a modifier for this though, instead of detecting whether it has an input with screen reader text, since that's pretty fragile.
…ng-sullyman/patternfly into card-unified-theme-token-updates
|
closing in favor of #8219 |
|
Closing as we merged #8219 instead |
8 participants
Closes #8086
Figma: https://www.figma.com/design/qRn3S225WwGHIdgGZX0LTC/Unified-Theme--Design-Tokens--Styles---Specs?node-id=9552-14258&p=f&t=AmaPrRnVNe38gCx6-0
Convenience link: https://pf-pr-8180.surge.sh/components/card
Summary by CodeRabbit