feat(icon): apply rh-standard styling and add opt-out#12320
feat(icon): apply rh-standard styling and add opt-out#12320kmcfaul wants to merge 3 commits intopatternfly:mainfrom
Conversation
kmcfaul
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdded 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. |
3 participants
What: Closes #12321
noStandardSetStylingto opt out of applying the classSummary by CodeRabbit