Skip to content

Commit 726063b

Browse files
tlabajkmcfaul
tlabaj
and
kmcfaul
authored
fix(createIcon): Fix broken API in createIcon.tsx (#12336)
* fix(createIcon): Fix broken API in createIcon.tsx * updates to address coderabbit comments * updates from pr review * fixes from review comments * more fixes from review comments * more fixes from review comments * more fixes from review comments * more fixes from review comments * more clean up * more clean up * cleaned up IconDefinition vs IconData, reverted createIcon props interface rename, updated comments --------- Co-authored-by: Katie McFaul <kmcfaul@redhat.com>
1 parent 6b93dbd commit 726063b

4 files changed

Lines changed: 199 additions & 67 deletions

File tree

‎packages/react-icons/scripts/writeIcons.mjs‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,9 @@ import { pfToRhIcons } from './icons/pfToRhIcons.mjs';
77
import * as url from 'url';
88
const __dirname = url.fileURLToPath(new URL('.', import.meta.url));
99

10-
// Import createIcon from compiled dist (build:esm must run first)
10+
// Import createIconBase from compiled dist (build:esm must run first)
1111
const createIconModule = await import('../dist/esm/createIcon.js');
12-
const createIcon = createIconModule.createIcon;
12+
const createIconBase = createIconModule.createIconBase;
1313

1414
const outDir = join(__dirname, '../dist');
1515
const staticDir = join(outDir, 'static');
@@ -27,7 +27,7 @@ exports.${jsName}Config = {
2727
icon: ${JSON.stringify(icon)},
2828
rhUiIcon: ${rhUiIcon ? JSON.stringify(rhUiIcon) : 'null'},
2929
};
30-
exports.${jsName} = require('../createIcon').createIcon(exports.${jsName}Config);
30+
exports.${jsName} = require('../createIcon').createIconBase(exports.${jsName}Config);
3131
exports["default"] = exports.${jsName};
3232
`.trim()
3333
);
@@ -36,15 +36,15 @@ exports["default"] = exports.${jsName};
3636
const writeESMExport = (fname, jsName, icon, rhUiIcon = null) => {
3737
outputFileSync(
3838
join(outDir, 'esm/icons', `${fname}.js`),
39-
`import { createIcon } from '../createIcon.js';
39+
`import { createIconBase } from '../createIcon.js';
4040
4141
export const ${jsName}Config = {
4242
name: '${jsName}',
4343
icon: ${JSON.stringify(icon)},
4444
rhUiIcon: ${rhUiIcon ? JSON.stringify(rhUiIcon) : 'null'},
4545
};
4646
47-
export const ${jsName} = createIcon(${jsName}Config);
47+
export const ${jsName} = createIconBase(${jsName}Config);
4848
4949
export default ${jsName};
5050
`.trim()
@@ -68,16 +68,16 @@ export default ${jsName};
6868
};
6969

7070
/**
71-
* Generates a static SVG string from icon data using createIcon
71+
* Generates a static SVG string from icon data using createIconBase
7272
* @param {string} iconName The name of the icon
7373
* @param {object} icon The icon data object
7474
* @returns {string} Static SVG markup
7575
*/
7676
function generateStaticSVG(iconName, icon) {
7777
const jsName = `${toCamel(iconName)}Icon`;
7878

79-
// Create icon component using createIcon
80-
const IconComponent = createIcon({
79+
// Create icon component using createIconBase
80+
const IconComponent = createIconBase({
8181
name: jsName,
8282
icon
8383
});

‎packages/react-icons/src/__tests__/createIcon.test.tsx‎

Lines changed: 114 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
import { render, screen } from '@testing-library/react';
2-
import { IconDefinition, CreateIconProps, createIcon, SVGPathObject } from '../createIcon';
2+
import { IconDefinition, CreateIconProps, createIcon, createIconBase, SVGPathObject } from '../createIcon';
3+
4+
/** Mirrors the non-exported argument type of {@link createIconBase} for tests. */
5+
type CreateIconBaseProps = Parameters<typeof createIconBase>[0];
36

47
const multiPathIcon: IconDefinition = {
58
name: 'IconName',
@@ -28,24 +31,24 @@ const rhStandardIcon: IconDefinition = {
2831
svgClassName: 'pf-v6-icon-rh-standard'
2932
};
3033

31-
const iconDef: CreateIconProps = {
34+
const iconDef: CreateIconBaseProps = {
3235
name: 'SinglePathIconName',
3336
icon: singlePathIcon
3437
};
3538

36-
const iconDefWithArrayPath: CreateIconProps = {
39+
const iconDefWithArrayPath: CreateIconBaseProps = {
3740
name: 'MultiPathIconName',
3841
icon: multiPathIcon
3942
};
4043

41-
const iconDefWithRhStandard: CreateIconProps = {
44+
const iconDefWithRhStandard: CreateIconBaseProps = {
4245
name: 'RhStandardIconName',
4346
icon: rhStandardIcon
4447
};
4548

46-
const SVGIcon = createIcon(iconDef);
47-
const SVGArrayIcon = createIcon(iconDefWithArrayPath);
48-
const RhStandardIcon = createIcon(iconDefWithRhStandard);
49+
const SVGIcon = createIconBase(iconDef);
50+
const SVGArrayIcon = createIconBase(iconDefWithArrayPath);
51+
const RhStandardIcon = createIconBase(iconDefWithRhStandard);
4952

5053
test('sets correct viewBox', () => {
5154
render(<SVGIcon />);
@@ -57,7 +60,23 @@ test('sets correct viewBox', () => {
5760

5861
test('sets correct svgPath if string', () => {
5962
render(<SVGIcon />);
60-
expect(screen.getByRole('img', { hidden: true }).querySelector('path')).toHaveAttribute('d', iconDef.svgPath);
63+
expect(screen.getByRole('img', { hidden: true }).querySelector('path')).toHaveAttribute(
64+
'd',
65+
singlePathIcon.svgPathData
66+
);
67+
});
68+
69+
test('accepts flat createIcon({ svgPath }) shape', () => {
70+
const legacyDef: CreateIconProps = {
71+
name: 'LegacyIcon',
72+
width: 10,
73+
height: 20,
74+
svgPath: 'legacy-path',
75+
svgClassName: 'legacy-svg'
76+
};
77+
const LegacySVGIcon = createIcon(legacyDef);
78+
render(<LegacySVGIcon />);
79+
expect(screen.getByRole('img', { hidden: true }).querySelector('path')).toHaveAttribute('d', 'legacy-path');
6180
});
6281

6382
test('sets correct svgClassName by default', () => {
@@ -127,3 +146,90 @@ test('additional props should be spread to the root svg element', () => {
127146
render(<SVGIcon data-testid="icon" />);
128147
expect(screen.getByTestId('icon')).toBeInTheDocument();
129148
});
149+
150+
describe('rh-ui mapping: nested SVGs, set prop, and warnings', () => {
151+
const defaultPath = 'M0 0-default';
152+
const rhUiPath = 'M0 0-rh-ui';
153+
154+
const defaultIconDef: IconDefinition = {
155+
name: 'DefaultVariant',
156+
width: 16,
157+
height: 16,
158+
svgPathData: defaultPath
159+
};
160+
161+
const rhUiIconDef: IconDefinition = {
162+
name: 'RhUiVariant',
163+
width: 16,
164+
height: 16,
165+
svgPathData: rhUiPath
166+
};
167+
168+
const dualConfig: CreateIconBaseProps = {
169+
name: 'DualMappedIcon',
170+
icon: defaultIconDef,
171+
rhUiIcon: rhUiIconDef
172+
};
173+
174+
const DualMappedIcon = createIconBase(dualConfig);
175+
176+
test('renders two nested inner svgs when rhUiIcon is set and `set` is omitted (swap layout)', () => {
177+
render(<DualMappedIcon />);
178+
const root = screen.getByRole('img', { hidden: true });
179+
expect(root).toHaveClass('pf-v6-svg');
180+
const innerSvgs = root.querySelectorAll(':scope > svg');
181+
expect(innerSvgs).toHaveLength(2);
182+
expect(root?.querySelector('.pf-v6-icon-default path')).toHaveAttribute('d', defaultPath);
183+
expect(root?.querySelector('.pf-v6-icon-rh-ui path')).toHaveAttribute('d', rhUiPath);
184+
});
185+
186+
test('set="default" renders a single flat svg using the default icon paths', () => {
187+
render(<DualMappedIcon set="default" />);
188+
const root = screen.getByRole('img', { hidden: true });
189+
expect(root.querySelectorAll(':scope > svg')).toHaveLength(0);
190+
expect(root).toHaveAttribute('viewBox', '0 0 16 16');
191+
expect(root.querySelector('path')).toHaveAttribute('d', defaultPath);
192+
expect(root.querySelectorAll('svg')).toHaveLength(0);
193+
});
194+
195+
test('set="rh-ui" renders a single flat svg using the rh-ui icon paths', () => {
196+
render(<DualMappedIcon set="rh-ui" />);
197+
const root = screen.getByRole('img', { hidden: true });
198+
expect(root.querySelectorAll(':scope > svg')).toHaveLength(0);
199+
expect(root.querySelector('path')).toHaveAttribute('d', rhUiPath);
200+
expect(root.querySelectorAll('svg')).toHaveLength(0);
201+
});
202+
203+
test('set="rh-ui" with no rhUiIcon mapping falls back to default and warns', () => {
204+
const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
205+
try {
206+
const IconNoRhMapping = createIconBase({
207+
name: 'NoRhMappingIcon',
208+
icon: defaultIconDef,
209+
rhUiIcon: null
210+
});
211+
212+
render(<IconNoRhMapping set="rh-ui" />);
213+
214+
expect(warnSpy).toHaveBeenCalledWith(
215+
'Set "rh-ui" was provided for NoRhMappingIcon, but no rh-ui icon data exists for this icon. The default icon will be rendered.'
216+
);
217+
const root = screen.getByRole('img', { hidden: true });
218+
expect(root.querySelector('path')).toHaveAttribute('d', defaultPath);
219+
expect(root.querySelectorAll('svg')).toHaveLength(0);
220+
} finally {
221+
warnSpy.mockRestore();
222+
}
223+
});
224+
225+
test('warns when createIconBase omits icon', () => {
226+
const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
227+
createIconBase({
228+
name: 'MissingDefaultIcon',
229+
rhUiIcon: null
230+
});
231+
expect(warnSpy).toHaveBeenCalledWith(
232+
'@patternfly/react-icons: createIconBase is missing an `icon` definition (name: MissingDefaultIcon).'
233+
);
234+
});
235+
});

0 commit comments

Comments
 (0)