feat(label): migrate label icons to RH set and update gap token - #8181
feat(label): migrate label icons to RH set and update gap token#8181tarunvashishth wants to merge 9 commits into
Conversation
WalkthroughThe Label component now renders RH icons through ChangesLabel icon rendering and usage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/patternfly/components/Label/examples/Label.cssParsing error: Private names are only allowed in property accesses ( 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-8181.surge.sh A11y report: https://pf-pr-8181-a11y.surge.sh |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/Label/label--variants.hbs`:
- Around line 64-65: Replace the semantically incorrect icon value used for
neutral/object label examples: update every occurrence of
label-icon--value="rh-ui-add-circle-fill" in label--variants.hbs to
label-icon--value="rh-ui-cubes-fill" (this preserves the original neutral/object
semantics of the former "cube" examples — check the instances of the
label-icon--value attribute in the label--variants--title/icon variants).
In `@src/patternfly/demos/Card/templates/card-demo--notification-drawer.hbs`:
- Around line 33-36: The teal notification label reuses the same attribute value
label--id="default-blue-icon", causing duplicate IDs; update the second usage to
a unique id (e.g., label--id="default-teal-icon" or another distinct identifier)
in the Handlebars template where the notification label attributes are defined
(the element with label--id, label--color="teal", label-text--value="3",
label-icon--value="rh-ui-notification-fill") so each label has a unique
label--id within the DOM scope.
In `@src/patternfly/demos/Card/templates/card-template-status.hbs`:
- Around line 112-117: The orange notification label uses the error icon; update
the label partial so the label with label--id="default-orange-icon" and
label--color="orange" uses a warning icon instead of the error icon by replacing
the label-icon--value="rh-ui-error-fill" with the appropriate warning icon value
(e.g., label-icon--value="rh-icon-warning-fill") to keep semantics consistent.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
src/patternfly/components/Label/examples/Label.mdsrc/patternfly/components/Label/label--variants.hbssrc/patternfly/components/Label/label-group-close.hbssrc/patternfly/components/Label/label-icon--map.hbssrc/patternfly/components/Label/label-icon.hbssrc/patternfly/components/Label/label.hbssrc/patternfly/components/Label/label.scsssrc/patternfly/components/Toolbar/templates/toolbar--item-label-group.hbssrc/patternfly/demos/Card/examples/Card.mdsrc/patternfly/demos/Card/templates/card-demo--notification-drawer.hbssrc/patternfly/demos/Card/templates/card-template-status.hbssrc/patternfly/demos/Form/examples/BasicForms.mdsrc/patternfly/demos/Tabs/tabs--pod-page-title.hbs
| label-icon--value="rh-ui-add-circle-fill" | ||
| label-text--value=(concat label--variants--title ' icon')}} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Font Awesome cube icon meaning semantics what does it represent
💡 Result:
In Font Awesome, the “cube” / “cubes” icons are primarily literal pictograms (a 3D cube, or a small stack of cubes). Font Awesome doesn’t assign a single fixed “semantic meaning” beyond the visual metaphor, so what it represents depends on your UI context. (icon-sets.iconify.design)
Common, widely-understood uses:
- Package / box / shipping (a “boxed” item)
- Product / inventory item / SKU
- 3D / modeling / CAD / VR (a basic 3D primitive)
- Modules / building blocks / components (especially for “cubes” / stacked cubes)
- Data blocks / blockchain / containers (metaphor: discrete units)
Accessibility note: if the cube icon conveys meaning (not just decoration), give it an accessible name (e.g., accompanying text, or an aria-label). Font Awesome’s own examples note using aria-hidden="true" when an icon is purely decorative. (fontawesome.com)
Citations:
- 1: https://icon-sets.iconify.design/fa/cube/?utm_source=openai
- 2: https://fontawesome.com/v4.7.0/icon/cubes?utm_source=openai
🌐 Web query:
Red Hat PatternFly rh-ui-add-circle-fill icon meaning purpose
💡 Result:
In PatternFly, rh-ui-add-circle-fill (aka RhUiAddCircleFillIcon) is a Red Hat “UI” icon: a plus (“add”) symbol inside a filled circle, intended to communicate an add/create action (e.g., “add item”, “create new”, “add to list”) in interactive UI contexts. UI icons are meant to be immediately understood at small sizes and used to facilitate navigation or provide UI context. [1]
How it’s used in code:
- PatternFly React icon component name:
RhUiAddCircleFillIcon[2] - Red Hat Design System
<rh-icon>usage:<rh-icon set="ui" icon="add-circle-fill"></rh-icon>[3]
Why the “circle-fill” variant exists: it’s the circled, filled style of the base “add” icon, typically chosen when you want a more prominent, button-like standalone add affordance (vs. a plain plus). [2][3]
Sources
[1] https://ux.redhat.com/elements/icon/guidelines/
[2] https://pf-react-staging.patternfly.org/icons/
[3] https://red-hat-icons.netlify.app/
🌐 Web query:
Red Hat PatternFly rh-ui-cubes-fill icon semantics
💡 Result:
rh-ui-cubes-fill (React: RhUiCubesFillIcon) is a Red Hat “UI” icon in PatternFly’s Red Hat icon set; it’s simply the filled/solid variant of rh-ui-cubes (“cubes”). There isn’t an additional, formally documented “semantic meaning” attached to this specific glyph beyond the visual metaphor implied by its name (i.e., “cubes” → a group/collection of modular items/objects/components) and whatever meaning your product assigns in-context. [1], [2]
For accessibility, follow the general Red Hat UI icon guidance: use UI icons as small, utilitarian interface symbols and provide an appropriate accessible name/label based on the actual action/object in your UI, not the icon filename. [3]
Sources: [1] PatternFly React icons list (RhUiCubesFillIcon), [2] Red Hat Icons gallery (UI set: cubes-fill), [3] Red Hat brand standards: UI icons guidance.
🌐 Web query:
Red Hat PatternFly rh-ui-package-fill icon semantics neutral object
💡 Result:
In PatternFly’s Red Hat UI icon set, rh-ui-package-fill is the filled “Package” icon—a neutral, object-representational symbol (a package/box), typically used to denote a package/artifact/bundle (e.g., a software package, deliverable, or packaged item) rather than a status (success/warning/error) or an action. (pf-react-staging.patternfly.org)
Citations:
cube → rh-ui-add-circle-fill is semantically mismatched for neutral label icon usage.
Lines 64/76/104/111/119/127 replace a neutral object icon (Font Awesome's cube, commonly representing packages/items/components) with an action icon (add-circle-fill, which explicitly communicates "add/create"). This misalignment changes the example semantics from object reference to action intent, violating the "closest RH replacement" principle.
Use rh-ui-cubes-fill instead—it preserves the neutral, modular-object semantics of the original cube icon.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/components/Label/label--variants.hbs` around lines 64 - 65,
Replace the semantically incorrect icon value used for neutral/object label
examples: update every occurrence of label-icon--value="rh-ui-add-circle-fill"
in label--variants.hbs to label-icon--value="rh-ui-cubes-fill" (this preserves
the original neutral/object semantics of the former "cube" examples — check the
instances of the label-icon--value attribute in the label--variants--title/icon
variants).
| {{> label | ||
| label--id="default-orange-icon" | ||
| label--color="orange" | ||
| label-text--value="1" | ||
| label-icon--value="exclamation-circle"}} | ||
| label-icon--value="rh-ui-error-fill"}} | ||
| {{/label-group-list-item}} |
There was a problem hiding this comment.
Use warning icon for the orange notification label.
At Line 116, label--color="orange" is paired with rh-ui-error-fill; this should be the warning icon for semantic consistency.
Proposed fix
{{> label
label--id="default-orange-icon"
label--color="orange"
label-text--value="1"
- label-icon--value="rh-ui-error-fill"}}
+ label-icon--value="rh-ui-warning-fill"}}📝 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.
| {{> label | |
| label--id="default-orange-icon" | |
| label--color="orange" | |
| label-text--value="1" | |
| label-icon--value="exclamation-circle"}} | |
| label-icon--value="rh-ui-error-fill"}} | |
| {{/label-group-list-item}} | |
| {{> label | |
| label--id="default-orange-icon" | |
| label--color="orange" | |
| label-text--value="1" | |
| label-icon--value="rh-ui-warning-fill"}} | |
| {{/label-group-list-item}} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/demos/Card/templates/card-template-status.hbs` around lines
112 - 117, The orange notification label uses the error icon; update the label
partial so the label with label--id="default-orange-icon" and
label--color="orange" uses a warning icon instead of the error icon by replacing
the label-icon--value="rh-ui-error-fill" with the appropriate warning icon value
(e.g., label-icon--value="rh-icon-warning-fill") to keep semantics consistent.
mcoker
left a comment
There was a problem hiding this comment.
Thanks @tarunvashishth! This looks great, however looks like our designs show that compact labels still have the smaller font/icon sizes and gaps. Do you mind updating this PR with those changes? I believe I called out everything needed.
| --#{$label}--m-compact--PaddingBlockEnd: 0; | ||
| --#{$label}--m-compact--PaddingInlineStart: var(--pf-t--global--spacer--sm); | ||
| --#{$label}--m-compact--FontSize: var(--pf-t--global--font--size--body--sm); | ||
| --#{$label}--m-compact--FontSize: var(--pf-t--global--font--size--body--default); |
There was a problem hiding this comment.
Per figma, looks like the font-size for compact labels stays the same
| --#{$label}--m-compact--FontSize: var(--pf-t--global--font--size--body--default); | |
| --#{$label}--m-compact--FontSize: var(--pf-t--global--font--size--body--sm); |
|
|
||
| // Icon | ||
| --#{$label}__icon--FontSize: var(--pf-t--global--icon--size--font--body--sm); | ||
| --#{$label}__icon--FontSize: var(--pf-t--global--icon--size--font--body--default); |
There was a problem hiding this comment.
Need to create var for compact icon size and set to --pf-t--global--icon--size--font--body--sm
|
|
||
| // Actions button (close) | ||
| --#{$label}__actions--c-button--FontSize: var(--pf-t--global--icon--size--font--body--sm); | ||
| --#{$label}__actions--c-button--FontSize: var(--pf-t--global--icon--size--font--body--default); |
There was a problem hiding this comment.
Need to create var for compact icon size and set to --pf-t--global--icon--size--font--body--sm
| --#{$label}--FontSize: var(--pf-t--global--font--size--body--sm); | ||
| --#{$label}--Gap: var(--pf-t--global--spacer--gap--text-to-element--compact); | ||
| --#{$label}--FontSize: var(--pf-t--global--font--size--body--default); | ||
| --#{$label}--Gap: var(--pf-t--global--spacer--gap--text-to-element--default); |
There was a problem hiding this comment.
Need to introduce a compact gap set to the text-to-element--compact var
| // Content | ||
| --#{$label}__content--MaxWidth: 100%; | ||
| --#{$label}__content--Gap: var(--pf-t--global--spacer--gap--text-to-element--compact); | ||
| --#{$label}__content--Gap: var(--pf-t--global--spacer--gap--text-to-element--default); |
There was a problem hiding this comment.
Need to create a compact var for this gap, too, set to text-to-element--compact
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/Label/label.scss`:
- Around line 208-209: The default label gap variable --#{$label}--Gap is
incorrectly using the compact token; change its value from
var(--pf-t--global--spacer--gap--text-to-element--compact) to the default
var(--pf-t--global--spacer--gap--text-to-element) so regular labels get the 8px
spacing, and leave the compact-specific variables (--#{$label}--m-compact--Gap
and --#{$label}--m-compact__content--Gap) using the compact token; make the same
replacement for the other occurrence of --#{$label}--Gap noted around the later
block (the pair at lines 435-436).
🪄 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: 7a496f35-a976-44a7-8bc1-06c0eeb28a5b
📒 Files selected for processing (1)
src/patternfly/components/Label/label.scss
| --#{$label}--m-compact--Gap: var(--pf-t--global--spacer--gap--text-to-element--compact); | ||
| --#{$label}--m-compact__content--Gap: var(--pf-t--global--spacer--gap--text-to-element--compact); |
There was a problem hiding this comment.
Use the default text-to-element gap for regular labels.
These compact vars/wiring are good, but regular label spacing is still compact because --#{$label}--Gap at Line 14 remains text-to-element--compact. That misses the objective for default (8px) text-to-close spacing.
Proposed fix
- --#{$label}--Gap: var(--pf-t--global--spacer--gap--text-to-element--compact);
+ --#{$label}--Gap: var(--pf-t--global--spacer--gap--text-to-element--default);Also applies to: 435-436
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/components/Label/label.scss` around lines 208 - 209, The
default label gap variable --#{$label}--Gap is incorrectly using the compact
token; change its value from
var(--pf-t--global--spacer--gap--text-to-element--compact) to the default
var(--pf-t--global--spacer--gap--text-to-element) so regular labels get the 8px
spacing, and leave the compact-specific variables (--#{$label}--m-compact--Gap
and --#{$label}--m-compact__content--Gap) using the compact token; make the same
replacement for the other occurrence of --#{$label}--Gap noted around the later
block (the pair at lines 435-436).
8a27b87 to
a34ec73
Compare
|
@bekah-stephens @andrew-ronaldson here is what this PR does:
Here is the visual regression report if you want to see how it impacted any of examples/demos |
bac7a61 to
81bb04f
Compare
|
@andrew-ronaldson @bekah-stephens updated per our meeting this morning to:
Also the border colors change for filled labels now. I hadn't pushed my latest changes when we looked this morning.
|
f3e5d34 to
8bf5649
Compare
8bf5649 to
6dca332
Compare
|
Per meeting with design, putting this PR on hold for Q2. We need some clarification around:
|
jcmill
left a comment
There was a problem hiding this comment.
Thanks for taking this on @tarunvashishth. Quite the task and we still need some clarification on a few of the things Michael pointed out. I'll trying and bring those up in our next meeting. Apart from that, I added a couple more things, apologies to the mountain this one seems to be creating.
| @@ -1,9 +1,9 @@ | |||
| {{#if label-icon--value}} | |||
| {{~label-icon--value}} | |||
| {{~pfIcon label-icon--value}} | |||
There was a problem hiding this comment.
The ~ in this and the ifEquals aren't really needed anymore because we aren't passing stings into classes anymore.
| label--color="orange" | ||
| label-text--value="1" | ||
| label-icon--value="exclamation-circle"}} | ||
| label-icon--value="rh-ui-error-fill"}} |
d538704 to
9ca2c69
Compare
- remove unneeded whitespace control (~) from label-icon--map - fix duplicate label--id on the teal notification label
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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/Label/label.scss`:
- Around line 733-747: Update the clicked-state selector in the &.pf-m-clicked
block so it targets the child .#{$label}__actions element rendered within the
Label, after .#{$label}__content, rather than using a sibling combinator from
the Label root. Preserve the existing filled close-button icon color variable
assignment.
🪄 Autofix
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 Plus
Run ID: 0287bede-8673-46f2-85d2-1d606150c48f
📒 Files selected for processing (11)
src/patternfly/components/Label/examples/Label.csssrc/patternfly/components/Label/examples/Label.mdsrc/patternfly/components/Label/label--variants.hbssrc/patternfly/components/Label/label-icon--map.hbssrc/patternfly/components/Label/label-icon.hbssrc/patternfly/components/Label/label.hbssrc/patternfly/components/Label/label.scsssrc/patternfly/demos/Card/examples/Card.mdsrc/patternfly/demos/Card/templates/card-demo--notification-drawer.hbssrc/patternfly/demos/Form/examples/BasicForms.mdsrc/patternfly/demos/Tabs/tabs--pod-page-title.hbs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/patternfly/demos/Card/examples/Card.md
- src/patternfly/demos/Form/examples/BasicForms.md
- src/patternfly/demos/Tabs/tabs--pod-page-title.hbs
- src/patternfly/components/Label/label-icon--map.hbs
| &.pf-m-clicked { | ||
| .#{$label}__content, | ||
| .#{$label}__content:is(:hover, :focus) { | ||
| --#{$label}--Color: var(--#{$label}--m-clicked--Color); | ||
| --#{$label}__icon--Color: var(--#{$label}--m-clicked__icon--Color); | ||
| --#{$label}--BackgroundColor: var(--#{$label}--m-clicked--BackgroundColor); | ||
| --#{$label}--BorderWidth: var(--#{$label}--m-clicked--BorderWidth); | ||
| --#{$label}--BorderColor: var(--#{$label}--m-clicked--BorderColor); | ||
| --#{$label}--m-outline--BorderWidth: var(--#{$label}--m-outline--m-clicked--BorderWidth); | ||
| --#{$label}--m-outline--BorderColor: var(--#{$label}--m-outline--m-clicked--BorderColor); | ||
| } | ||
|
|
||
| ~ .#{$label}__actions .#{$button} { | ||
| --#{$label}--m-filled__actions--c-button__icon--Color: var(--#{$label}--m-clicked__icon--Color); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Set the clicked close-button icon color on the child action.
The selector on Line 745 targets an .pf-v6-c-label__actions sibling of the Label root. label.hbs renders the actions element inside the Label, after .pf-v6-c-label__content. Therefore, this rule never matches and a filled removable clicked Label keeps its default close-icon color.
Proposed fix
- ~ .#{$label}__actions .#{$button} {
+ .#{$label}__content ~ .#{$label}__actions .#{$button} {
--#{$label}--m-filled__actions--c-button__icon--Color: var(--#{$label}--m-clicked__icon--Color);
}📝 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.
| &.pf-m-clicked { | |
| .#{$label}__content, | |
| .#{$label}__content:is(:hover, :focus) { | |
| --#{$label}--Color: var(--#{$label}--m-clicked--Color); | |
| --#{$label}__icon--Color: var(--#{$label}--m-clicked__icon--Color); | |
| --#{$label}--BackgroundColor: var(--#{$label}--m-clicked--BackgroundColor); | |
| --#{$label}--BorderWidth: var(--#{$label}--m-clicked--BorderWidth); | |
| --#{$label}--BorderColor: var(--#{$label}--m-clicked--BorderColor); | |
| --#{$label}--m-outline--BorderWidth: var(--#{$label}--m-outline--m-clicked--BorderWidth); | |
| --#{$label}--m-outline--BorderColor: var(--#{$label}--m-outline--m-clicked--BorderColor); | |
| } | |
| ~ .#{$label}__actions .#{$button} { | |
| --#{$label}--m-filled__actions--c-button__icon--Color: var(--#{$label}--m-clicked__icon--Color); | |
| } | |
| &.pf-m-clicked { | |
| .#{$label}__content, | |
| .#{$label}__content:is(:hover, :focus) { | |
| --#{$label}--Color: var(--#{$label}--m-clicked--Color); | |
| --#{$label}__icon--Color: var(--#{$label}--m-clicked__icon--Color); | |
| --#{$label}--BackgroundColor: var(--#{$label}--m-clicked--BackgroundColor); | |
| --#{$label}--BorderWidth: var(--#{$label}--m-clicked--BorderWidth); | |
| --#{$label}--BorderColor: var(--#{$label}--m-clicked--BorderColor); | |
| --#{$label}--m-outline--BorderWidth: var(--#{$label}--m-outline--m-clicked--BorderWidth); | |
| --#{$label}--m-outline--BorderColor: var(--#{$label}--m-outline--m-clicked--BorderColor); | |
| } | |
| .#{$label}__content ~ .#{$label}__actions .#{$button} { | |
| --#{$label}--m-filled__actions--c-button__icon--Color: var(--#{$label}--m-clicked__icon--Color); | |
| } |
🤖 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/Label/label.scss` around lines 733 - 747, Update
the clicked-state selector in the &.pf-m-clicked block so it targets the child
.#{$label}__actions element rendered within the Label, after
.#{$label}__content, rather than using a sibling combinator from the Label root.
Preserve the existing filled close-button icon color variable assignment.
closes #8128 and #8138
Summary by CodeRabbit
New Features
Style