feat(icons): add swap support for mapped rh-ui icons#12245
feat(icons): add swap support for mapped rh-ui icons#12245kmcfaul wants to merge 9 commits intopatternfly:mainfrom
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:
WalkthroughAdds dynamic icon swapping: introduces a PF→RH-UI icon mapping, threads alternative Changes
Sequence DiagramsequenceDiagram
participant Build as Build System
participant Mapping as pfToRhIcons
participant Script as writeIcons.mjs
participant Registry as Generated Exports
participant Component as createIcon
participant Runtime as SVGIcon
Build->>Mapping: import pfToRhIcons
Mapping-->>Build: mapping entries (rhUiIcon or null)
Build->>Script: run writeIcons for icon list
Script->>Mapping: lookup pfToRhIcons[jsName]
Mapping-->>Script: return rhUiIcon (or null)
Script->>Registry: emit icon configs including rhUiIcon
Registry-->>Component: createIcon invoked with icon + rhUiIcon
Component->>Runtime: produce SVGIcon supporting 'set' prop
Runtime->>Runtime: 'set' selects PF or RH‑UI paths for rendering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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-12245.surge.sh A11y report: https://pf-react-pr-12245-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: 1
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/__tests__/createIcon.test.tsx (1)
44-47:⚠️ Potential issue | 🔴 CriticalBug:
iconDef.svgPathisundefined— test will fail or pass vacuously.
iconDefis now typed asCreateIconPropswith shape{ name, icon }. There is nosvgPathproperty on it, soiconDef.svgPathevaluates toundefined. This test either fails or incorrectly passes depending on howtoHaveAttributehandlesundefined.It should reference the path data from the underlying
IconDefinition:🐛 Proposed fix
test('sets correct svgPath if string', () => { render(<SVGIcon />); - expect(screen.getByRole('img', { hidden: true }).querySelector('path')).toHaveAttribute('d', iconDef.svgPath); + expect(screen.getByRole('img', { hidden: true }).querySelector('path')).toHaveAttribute('d', singlePathIcon.svgPathData as string); });
🤖 Fix all issues with AI agents
In `@packages/react-icons/src/createIcon.tsx`:
- Around line 123-152: The else-branch contains dead conditional checks and
produces a boolean|string for the second arg to createSvg; simplify by treating
set as undefined and rhUiIcon as non-null in this branch: assign
defaultIconClassName = 'pf-v6-icon-default' and rhUiIconClassName =
'pf-v6-icon-rh-ui' unconditionally, remove shouldRenderDefault/shouldRenderRhUi
and their impossible set checks, and call createSvg(icon, defaultIconClassName)
and createSvg(rhUiIcon, rhUiIconClassName) so the second parameter is always a
string (refer to createIcon component and the createSvg invocations).
🧹 Nitpick comments (5)
packages/react-icons/src/pfToRhIcons.js (1)
3-4: Silentnullfor missing/misspelled icon names could mask build-time errors.If an rh-ui icon name in the mapping doesn't exist in
rhIconsUI,getIconDatasilently returnsnull, and the generated icon module will haverhUiIcon: null— indistinguishable from an icon that intentionally has no mapping. Consider logging a warning during the build when a mapped icon name yields no data.💡 Suggested improvement
-const getIconData = (iconName) => rhIconsUI[iconName] || null; +const getIconData = (iconName) => { + const data = rhIconsUI[iconName]; + if (!data) { + console.warn(`[pfToRhIcons] No rh-ui icon data found for "${iconName}"`); + } + return data || null; +};packages/react-icons/scripts/writeIcons.mjs (1)
54-68: Type declarations embed full icon data as literal types — consider using the type name instead.
writeDTSExportserializes the entirerhUiIconSVG path data as a JSON literal in.d.tsfiles (rhUiIcon: {"svgPathData":"M0 0...","width":512,...}). This inflates.d.tsfile sizes and provides no type-safety benefit over a named type. Consider using theIconDefinition | nulltype instead of the literal value.💡 Suggested change for the type declaration
const text = `import { ComponentClass } from 'react'; -import { SVGIconProps } from '../createIcon'; +import { SVGIconProps, IconDefinition } from '../createIcon'; export declare const ${jsName}Config: { name: '${jsName}', - icon: ${JSON.stringify(icon)}, - rhUiIcon: ${rhUiIcon ? JSON.stringify(rhUiIcon) : 'null'}, + icon: IconDefinition, + rhUiIcon: IconDefinition | null, };packages/react-icons/src/createIcon.tsx (3)
73-75: Unnecessary constructor — can be removed.This constructor only calls
super(props), which is the default behavior. Removing it reduces noise.Suggested diff
- constructor(props: SVGIconProps) { - super(props); - } -
87-92: Branching condition is correct but hard to parse — consider simplifying.Line 87
(set === undefined && rhUiIcon === null) || set !== undefinedis equivalent torhUiIcon === null || set !== undefined. Similarly, on Line 88,set !== undefined && set === 'rh-ui'can be justset === 'rh-ui'.Suggested simplification
- if ((set === undefined && rhUiIcon === null) || set !== undefined) { - const iconData = set !== undefined && set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon; + if (rhUiIcon === null || set !== undefined) { + const iconData = set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon;
89-105: Duplicated viewBox/path rendering logic — consider reusingcreateSvgor extracting shared helpers.Lines 89–105 replicate the exact viewBox computation and path-rendering logic already in
createSvg(Lines 33–55). If the if-branch needs a flat (non-nested) SVG, you could still extract the shared parts (viewBox calc, path rendering) into small helpers and compose them in both branches, reducing the maintenance surface.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/react-icons/tsconfig.json`:
- Around line 14-15: The include entry in this tsconfig references
"scripts/pfToRhIcons.mjs" which doesn't match the actual file location
("scripts/icons/pfToRhIcons.mjs") and .mjs files won't be picked up unless
allowJs is enabled; update the include to the correct path
("scripts/icons/pfToRhIcons.mjs") or remove the .mjs entry if you don't want it
type-checked, and if you do want TypeScript to check the .mjs file enable
allowJs in the base tsconfig so the file is actually processed.
🧹 Nitpick comments (3)
packages/react-icons/scripts/icons/pfToRhIcons.mjs (2)
3-4: Silentnullfor missing icons may hide typos in mappings.
getIconDatareturnsnullwhen an icon name isn't found inrhIconsUI. If any of the ~170 active mapping strings contain a typo, the build will silently produce anulliconfield — resulting in a broken rh-ui icon at runtime with no build-time feedback.Consider logging a warning (or throwing during the build) when a lookup fails:
Proposed change
-const getIconData = (iconName) => rhIconsUI[iconName] || null; +const getIconData = (iconName) => { + const data = rhIconsUI[iconName]; + if (!data) { + console.warn(`[pfToRhIcons] rh-ui icon "${iconName}" not found in rhIconsUI`); + } + return data || null; +};
8-223:nameis always identical to thegetIconDataargument — consider DRY helper.Every entry repeats the rh-ui icon name twice (once in
name, once ingetIconData(…)). A small helper would eliminate the duplication and the risk of the two values drifting apart:Proposed refactor
+const rhUiEntry = (rhUiName) => ({ name: rhUiName, icon: getIconData(rhUiName) }); + export const pfToRhIcons = { - AddCircleOIcon: { name: 'rh-ui-add-circle', icon: getIconData('rh-ui-add-circle') }, - AngleDoubleLeftIcon: { name: 'rh-ui-double-caret-left', icon: getIconData('rh-ui-double-caret-left') }, + AddCircleOIcon: rhUiEntry('rh-ui-add-circle'), + AngleDoubleLeftIcon: rhUiEntry('rh-ui-double-caret-left'), // ... same pattern for all entriespackages/react-icons/scripts/writeIcons.mjs (1)
124-128: Simplify with optional chaining + nullish coalescing.Minor: the ternary can be replaced with the more concise form that the rest of the codebase seems to favor.
Proposed change
- const altIcon = pfToRhIcons[jsName] ? pfToRhIcons[jsName].icon : null; + const altIcon = pfToRhIcons[jsName]?.icon ?? null;
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/react-icons/src/createIcon.tsx`:
- Line 88: When consumers pass set === 'rh-ui' but no RH-UI mapping exists
(rhUiIcon === null) the code silently falls back to the default icon; update the
logic in createIcon (around the iconData computation) to emit a development-only
warning (e.g., check process.env.NODE_ENV !== 'production' or __DEV__) that
console.warns something like "Requested RH-UI icon mapping not found for <icon
name> — falling back to default" when set === 'rh-ui' && rhUiIcon === null, then
proceed to use the existing fallback assignment for iconData.
🧹 Nitpick comments (3)
packages/react-icons/src/createIcon.tsx (3)
87-122: Duplicated SVG-building logic betweencreateSvgand the if-branch.Lines 89–105 manually replicate the viewBox calculation and path rendering that
createSvg(lines 33–61) already encapsulates. The only difference is that here the result isn't wrapped in an inner<svg>. Consider extracting the shared path-rendering and viewBox logic into a common helper, or restructuring so the if-branch can also leveragecreateSvg(perhaps by havingcreateSvgreturn a fragment and applyingviewBoxat the call site).This would eliminate the maintenance burden of keeping two copies in sync.
87-87: Simplify the branching condition for clarity.
(set === undefined && rhUiIcon === null) || set !== undefinedis logically equivalent toset !== undefined || rhUiIcon === null. The current form is harder to parse at a glance.Suggested simplification
- if ((set === undefined && rhUiIcon === null) || set !== undefined) { + if (set !== undefined || rhUiIcon === null) {
73-75: No-op constructor can be removed.This constructor only calls
super(props)— the default behavior. Removing it reduces noise.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/react-icons/src/createIcon.tsx (4)
73-75: Unnecessary constructor.A constructor that only calls
super(props)is implicit in class components and can be removed.Suggested removal
- constructor(props: SVGIconProps) { - super(props); - } -
87-92: Warning fires on every render and is not gated to development mode.This
console.warnexecutes on every render cycle when the condition is met, which can flood the console. Additionally, it is not guarded by aprocess.env.NODE_ENV !== 'production'check, so it will appear in production builds.Gate warning to dev mode
if (set === 'rh-ui' && rhUiIcon === null) { + if (process.env.NODE_ENV !== 'production') { // eslint-disable-next-line no-console console.warn( `Set "rh-ui" was provided for ${name}, but no rh-ui icon data exists for this icon. The default icon will be rendered.` ); + } }
94-129: DRY violation: if-branch duplicatescreateSvglogic; condition can be simplified.Two concerns here:
Condition simplification:
(set === undefined && rhUiIcon === null) || set !== undefinedis logically equivalent toset !== undefined || rhUiIcon === null. The simpler form reads more clearly: "use a single SVG when asetis explicitly chosen or there's no rh-ui variant."Duplicated logic: Lines 96–112 replicate the same viewBox computation, className assembly, and path-rendering logic already encapsulated in
createSvg(lines 33–62). I understand the structural difference (flat vs. nested SVG), but at minimum the path-rendering portion (array-vs-string branch) could be extracted into a shared helper to avoid having two copies to maintain.Simplify condition and extract path rendering
- if ((set === undefined && rhUiIcon === null) || set !== undefined) { + if (set !== undefined || rhUiIcon === null) {For the path-rendering duplication, consider extracting a small helper:
const renderPaths = (svgPathData: string | SVGPathObject[]) => Array.isArray(svgPathData) ? svgPathData.map((pathObject, index) => ( <path className={pathObject.className} key={`${pathObject.path}-${index}`} d={pathObject.path} /> )) : (<path d={svgPathData as string} />);This could then be used in both the if-branch
renderand insidecreateSvg.
95-95: Redundantset !== undefinedguard.
set === 'rh-ui'already impliesset !== undefined, so the check can be shortened to:- const iconData = set !== undefined && set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon; + const iconData = set === 'rh-ui' && rhUiIcon !== null ? rhUiIcon : icon;
kmcfaul
requested review from
nicolethoen,
thatblindgeye and
wise-king-sullyman
2 participants
What: Closes #12220
Adds a mapping for a subset of existing PF icons with new corresponding RH-ui set icons.
Icon structure for mapped icons has been updated to the following nested structure (this does mean snapshot updates are required for icons in the mapping unless
setis specified):The displayed svg between
icon-defaultandicon-rh-uiis controlled via the newsetprop (which hasdefaultandrh-uivalues) or by applying thepf-v6-icon-set-rh-uiclass to thehtmlelement similar to theme.Summary by CodeRabbit
New Features
Bug Fixes / API
Chores