refactor: share icon ligature conversion across renderers#833
refactor: share icon ligature conversion across renderers#833stablegenius49 wants to merge 1 commit intogoogle:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request is a nice refactoring that centralizes the logic for converting camelCase icon names to snake_case for Material Symbols into a shared toMaterialSymbolLigature helper. This improves consistency and maintainability across the Angular, Lit, and React renderers. The changes are well-structured and include necessary configuration updates and tests. I have a few suggestions to improve the new helper function and its tests, and to simplify the TypeScript configuration for the React renderer.
| "paths": { | ||
| "@/*": ["./src/*"] | ||
| "@/*": ["./src/*"], | ||
| "@a2ui/web_core/styles/icons": ["../web_core/src/v0_8/styles/icons.ts"] |
There was a problem hiding this comment.
The path alias you've added here should be sufficient for TypeScript to resolve the module and its types directly from the source .ts file. The new declaration file renderers/react/src/web-core-icons.d.ts appears to be redundant and adds a maintenance burden, as it would need to be manually kept in sync with icons.ts.
Consider removing renderers/react/src/web-core-icons.d.ts and relying solely on the path mapping.
| assert.strictEqual(toMaterialSymbolLigature('shoppingCart'), 'shopping_cart'); | ||
| assert.strictEqual(toMaterialSymbolLigature('accountCircle'), 'account_circle'); |
There was a problem hiding this comment.
It would be good to add test cases for icon names that start with a capital letter to ensure they are converted correctly without a leading underscore, which would align with the suggested change in icons.ts.
assert.strictEqual(toMaterialSymbolLigature('shoppingCart'), 'shopping_cart');
assert.strictEqual(toMaterialSymbolLigature('ShoppingCart'), 'shopping_cart');
assert.strictEqual(toMaterialSymbolLigature('accountCircle'), 'account_circle');
assert.strictEqual(toMaterialSymbolLigature('AccountCircle'), 'account_circle');| * Material Symbols. | ||
| */ | ||
| export function toMaterialSymbolLigature(iconName: string): string { | ||
| return iconName.replace(/([A-Z])/g, '_$1').toLowerCase(); |
There was a problem hiding this comment.
The current implementation can produce a leading underscore if iconName starts with a capital letter (e.g., 'ShoppingCart' becomes '_shopping_cart'). Material Symbol ligatures don't typically start with an underscore.
You can fix this by removing the leading underscore after the replacement, before converting to lower case.
| return iconName.replace(/([A-Z])/g, '_$1').toLowerCase(); | |
| return iconName.replace(/([A-Z])/g, '_$1').replace(/^_/, '').toLowerCase(); |
Description
This consolidates the Material Symbols camelCase→snake_case conversion into a single
toMaterialSymbolLigature()helper inweb_core, then reuses it from the Lit and React icon renderers.It also brings the Angular icon renderer in line with the same behavior, so icon names like
shoppingCartandaccountCircleresolve consistently across all three renderers.Closes #786.
Pre-launch Checklist
Verification
node --test dist/src/v0_8/styles/icons.test.js(web_core helper test)npm test -- --run tests/unit/components/Icon.test.tsx(React icon tests)npm run typecheck(React)npm test(Lit)npm run build(Angular)