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 8 commits into
Conversation
WalkthroughReplaces Font Awesome icon identifiers with RH UI icons and switches label icon rendering to pfIcon/partial; updates close icon and compact label spacing/icon sizing across Label component templates, SCSS, toolbar items, and demos/examples. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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-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--id="default-blue-icon" | ||
| label--color="teal" | ||
| label-text--value="3" | ||
| label-icon--value="bell"}} | ||
| label-icon--value="rh-ui-notification-fill"}} |
There was a problem hiding this comment.
Fix duplicate label--id value in the notification labels.
Line 33 reuses label--id="default-blue-icon" for the teal label, creating duplicate IDs in the same DOM scope.
Proposed fix
{{> label
- label--id="default-blue-icon"
+ label--id="default-teal-icon"
label--color="teal"
label-text--value="3"
label-icon--value="rh-ui-notification-fill"}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/demos/Card/templates/card-demo--notification-drawer.hbs`
around lines 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.
There was a problem hiding this comment.
Sadly, I must agree with coderabbit. This should be changed to teal.
There was a problem hiding this comment.
@jcmill Thanks for confirming! The fix is straightforward — just update the teal label's ID:
{{> label
- label--id="default-blue-icon"
+ label--id="default-teal-icon"
label--color="teal"
label-text--value="3"
label-icon--value="rh-ui-notification-fill"}}(ᵔᴥᵔ)🐇
🧠 Learnings used
Learnt from: mcoker
Repo: patternfly/patternfly PR: 8206
File: src/patternfly/components/Button/notification-badge.hbs:13-19
Timestamp: 2026-03-06T22:30:46.870Z
Learning: In the patternfly/patternfly repository, icons rendered via {{pfIcon ...}} are produced by the pfIcon Handlebars helper (scripts/helpers.mjs) and their SVG output includes aria-hidden="true". Therefore, review comments claiming missing aria-hidden on icons rendered with {{pfIcon "..."}} in any .hbs template are false positives; no extra wrapping or aria-hidden attribute is needed for these pfIcon-rendered icons. If a template uses a raw SVG without aria-hidden, address that separately.
| {{> 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--id="default-blue-icon" | ||
| label--color="teal" | ||
| label-text--value="3" | ||
| label-icon--value="bell"}} | ||
| label-icon--value="rh-ui-notification-fill"}} |
There was a problem hiding this comment.
Sadly, I must agree with coderabbit. This should be changed to teal.
| label--color="orange" | ||
| label-text--value="1" | ||
| label-icon--value="exclamation-circle"}} | ||
| label-icon--value="rh-ui-error-fill"}} |
closes #8128 and #8138
Summary by CodeRabbit
Style
Refactor