feat(icon): apply rh-standard styling and add opt-out#12320
feat(icon): apply rh-standard styling and add opt-out#12320nicolethoen merged 4 commits intopatternfly:mainfrom
Conversation
kmcfaul
changed the title
WalkthroughAdds an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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-react-pr-12320.surge.sh A11y report: https://pf-react-pr-12320-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-icons/src/createIcon.tsx (1)
100-114:⚠️ Potential issue | 🟠 MajorOpt-out is not applied in the dual-render path.
noStandardSetStylingis only respected in the single-icon branch (Lines 107-114). In the dual-render branch (Lines 155-156),createSvg(...)still unconditionally appendssvgClassName, sopf-v6-icon-rh-standardcan still render even when opt-out is true.💡 Proposed fix
-const createSvg = (icon: IconDefinition, iconClassName: string) => { +const createSvg = (icon: IconDefinition, iconClassName: string, noStandardSetStyling = false) => { const { xOffset, yOffset, width, height, svgPathData, svgClassName } = icon ?? {}; @@ - if (svgClassName) { + if (svgClassName && !(noStandardSetStyling && svgClassName === 'pf-v6-icon-rh-standard')) { classNames.push(svgClassName); } @@ - if (svgClassName) { - if (svgClassName !== 'pf-v6-icon-rh-standard') { - classNames.push(svgClassName); - } else { - if (!noStandardSetStyling) { - classNames.push(svgClassName); - } - } - } + if (svgClassName && !(noStandardSetStyling && svgClassName === 'pf-v6-icon-rh-standard')) { + classNames.push(svgClassName); + } @@ - {icon && createSvg(icon, 'pf-v6-icon-default')} - {rhUiIcon && createSvg(rhUiIcon, 'pf-v6-icon-rh-ui')} + {icon && createSvg(icon, 'pf-v6-icon-default', noStandardSetStyling)} + {rhUiIcon && createSvg(rhUiIcon, 'pf-v6-icon-rh-ui', noStandardSetStyling)}Also applies to: 143-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-icons/src/createIcon.tsx` around lines 100 - 114, The dual-render path is appending svgClassName unconditionally when calling createSvg, so the opt-out flag noStandardSetStyling isn't respected and pf-v6-icon-rh-standard can still be applied; update the dual-render branch in createIcon/createSvg usage to mirror the single-icon logic: when svgClassName is defined, only push it to the class list if it's not 'pf-v6-icon-rh-standard' or if noStandardSetStyling is false (i.e., skip adding that specific class when noStandardSetStyling is true), ensuring the same conditional handling of svgClassName used for the single-icon branch (referencing svgClassName, noStandardSetStyling, createSvg, and iconData/rhUiIcon).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/react-icons/src/createIcon.tsx`:
- Around line 100-114: The dual-render path is appending svgClassName
unconditionally when calling createSvg, so the opt-out flag noStandardSetStyling
isn't respected and pf-v6-icon-rh-standard can still be applied; update the
dual-render branch in createIcon/createSvg usage to mirror the single-icon
logic: when svgClassName is defined, only push it to the class list if it's not
'pf-v6-icon-rh-standard' or if noStandardSetStyling is false (i.e., skip adding
that specific class when noStandardSetStyling is true), ensuring the same
conditional handling of svgClassName used for the single-icon branch
(referencing svgClassName, noStandardSetStyling, createSvg, and
iconData/rhUiIcon).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3406e8bd-ce94-4431-9d2e-d539acf6d03d
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
packages/react-core/package.jsonpackages/react-docs/package.jsonpackages/react-icons/package.jsonpackages/react-icons/scripts/icons/rhIconsStandard.mjspackages/react-icons/src/createIcon.tsxpackages/react-styles/package.jsonpackages/react-tokens/package.json
kmcfaul
requested review from
bekah-stephens,
thatblindgeye and
wise-king-sullyman
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Can we add some tests to the createIcon test file for this new prop?
kmcfaul
force-pushed
the
rh-standard-icon-styling
branch
from
278cbbe to
0ffa255
Compare
|
@thatblindgeye Updated. I had a thought as I was adding the unit tests though. Do you think we'd want the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-icons/src/__tests__/createIcon.test.tsx (1)
63-76: Add one regression test to protect non-standard class behavior.Current additions validate RH standard behavior, but a guard case for non-standard
svgClassNamewithnoStandardSetStyling={true}would prevent accidental broadening later.Suggested test addition
+test('does not affect non-standard svgClassName when noStandardSetStyling is true', () => { + render(<SVGIcon noStandardSetStyling />); + expect(screen.getByRole('img', { hidden: true })).toHaveClass(singlePathIcon.svgClassName ?? ''); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-icons/src/__tests__/createIcon.test.tsx` around lines 63 - 76, Add a regression test to ensure a custom svgClassName isn't overridden when noStandardSetStyling is true: render <RhStandardIcon noStandardSetStyling svgClassName="my-custom" /> and assert the rendered img (getByRole('img', { hidden: true })) hasClass('my-custom') and does not haveClass('pf-v6-icon-rh-standard'); this protects the non-standard class behavior in the RhStandardIcon component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-icons/src/__tests__/createIcon.test.tsx`:
- Around line 63-76: Add a regression test to ensure a custom svgClassName isn't
overridden when noStandardSetStyling is true: render <RhStandardIcon
noStandardSetStyling svgClassName="my-custom" /> and assert the rendered img
(getByRole('img', { hidden: true })) hasClass('my-custom') and does not
haveClass('pf-v6-icon-rh-standard'); this protects the non-standard class
behavior in the RhStandardIcon component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 079f5970-a69d-4429-a15f-beb7438d20e1
📒 Files selected for processing (3)
packages/react-icons/scripts/icons/rhIconsStandard.mjspackages/react-icons/src/__tests__/createIcon.test.tsxpackages/react-icons/src/createIcon.tsx
That would probably be nice, though not a blocker for me. |
|
@thatblindgeye I did update the prop to be more generic if you don't mind re-reviewing. I figured it simplifies the internal logic a good bit and that way other icons we add that may have default styling have an option to remove it. |
bekah-stephens
left a comment
bekah-stephens
left a comment
There was a problem hiding this comment.
looks good! (I think styling the Standard set differently is super helpful)
|
TODO - update parsing script to automatically re-add the classname to standard icons |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-icons/scripts/parseRHIcons.mjs (1)
21-26: SimplifyICON_TYPE_CLASSESto only include non-empty mappings.At Line 22 and Line 25, empty-string entries are no-ops. Keeping only actual class mappings reduces config drift and maintenance overhead.
Proposed simplification
const ICON_TYPE_CLASSES = { - ui: '', standard: 'pf-v6-icon-rh-standard', - // social: '', - microns: '' + // social: '' };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-icons/scripts/parseRHIcons.mjs` around lines 21 - 26, ICON_TYPE_CLASSES contains entries with empty-string values (e.g., keys "ui" and "microns") that are no-ops; remove those keys and keep only mappings with actual class names (e.g., retain "standard": "pf-v6-icon-rh-standard") so the configuration only contains meaningful class mappings—update any code that imports/reads ICON_TYPE_CLASSES to expect only the remaining keys if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-icons/scripts/parseRHIcons.mjs`:
- Around line 21-26: ICON_TYPE_CLASSES contains entries with empty-string values
(e.g., keys "ui" and "microns") that are no-ops; remove those keys and keep only
mappings with actual class names (e.g., retain "standard":
"pf-v6-icon-rh-standard") so the configuration only contains meaningful class
mappings—update any code that imports/reads ICON_TYPE_CLASSES to expect only the
remaining keys if necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38dd5390-a909-45da-ab86-e34f01bae516
📒 Files selected for processing (1)
packages/react-icons/scripts/parseRHIcons.mjs
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
rm -rf'd the dist/static dir and ran npm run update-rhds-icons && npm run generate and it generated new svgs and added the .pf-v6-icon-rh-standard to "rh-standard-*" icons. Also validated .pf-v6-icon-rh-standard isn't added to non-standard icons.
Wasn't able to start the dev server (complaining about classnames not in the styles object) to validate regular react-icons components work properly, but validated .pf-v6-icon-rh-standard is in the react-icons icon component data, and IIRC the static icons are created using the same script the react-icons components use to generate the svg so should be good!
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
7 participants
What: Closes #12321
noDefaultStyleto opt out of applying the classSummary by CodeRabbit