fix(renderers): add icon name overrides for v0.9 renderers#1146
fix(renderers): add icon name overrides for v0.9 renderers#1146ppazosp wants to merge 5 commits intogoogle:mainfrom
Conversation
Some A2UI spec icon names don't match Material Symbols ligatures (play, rewind, favoriteOff, starOff). Add a lookup table to all three v0.9 renderers (React, Angular, Lit) that maps these names to their correct Material Symbols equivalents before falling back to camelCase-to-snake_case conversion.
Test that the 4 icon name overrides (play, rewind, favoriteOff, starOff) resolve to their correct Material Symbols ligatures, and that camelCase-to-snake_case conversion works for multi-word icons.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces icon name overrides and camelCase to snake_case conversion across the Angular, Lit, and React renderers to ensure compatibility with Material Symbols. It also includes comprehensive tests for these changes. The review feedback identifies a logic error in the Lit and React implementations where the conversion function is incorrectly applied to SVG path strings, potentially corrupting the data. Additionally, suggestions were provided to optimize the string replacement logic in those renderers for better performance and consistency with the Angular implementation.
| const rawName = | ||
| typeof props.name === 'string' ? props.name : (props.name as {path?: string})?.path; | ||
| const iconName = rawName ? toSnakeCase(rawName) : undefined; | ||
| const iconName = rawName ? toMaterialSymbol(rawName) : undefined; |
There was a problem hiding this comment.
The toMaterialSymbol function (which performs snake_case conversion) is being applied to the icon name even when it represents an SVG path string. This occurs because rawName extracts the path string from the object before the conversion check. This is incorrect for SVG paths and inefficient.
| const rawName = | |
| typeof props.name === 'string' ? props.name : (props.name as {path?: string})?.path; | |
| const iconName = rawName ? toSnakeCase(rawName) : undefined; | |
| const iconName = rawName ? toMaterialSymbol(rawName) : undefined; | |
| const iconName = typeof props.name === 'string' | |
| ? toMaterialSymbol(props.name) | |
| : (props.name as {path?: string})?.path; |
There was a problem hiding this comment.
Fixed in 805fb5d. Now only applies toMaterialSymbol when props.name is a string.
| const name = | ||
| typeof props.name === "string" ? props.name : (props.name as any)?.path; | ||
| const iconName = name ? toMaterialSymbol(name) : name; |
There was a problem hiding this comment.
Similar to the React implementation, toMaterialSymbol should not be applied to SVG path strings. The conversion logic should only be triggered when the input is a string (representing an icon name).
| const name = | |
| typeof props.name === "string" ? props.name : (props.name as any)?.path; | |
| const iconName = name ? toMaterialSymbol(name) : name; | |
| const iconName = typeof props.name === "string" | |
| ? toMaterialSymbol(props.name) | |
| : (props.name as any)?.path; |
There was a problem hiding this comment.
Fixed in 805fb5d. Same approach as the React fix.
| function toSnakeCase(str: string): string { | ||
| return str.replace(/([A-Z])/g, '_$1').toLowerCase(); | ||
| function toMaterialSymbol(str: string): string { | ||
| return ICON_NAME_OVERRIDES[str] ?? str.replace(/([A-Z])/g, '_$1').toLowerCase(); |
There was a problem hiding this comment.
Consider using a more targeted replacement function to avoid capturing groups and lowercasing the entire string, aligning with the implementation in the Angular renderer for better efficiency.
| return ICON_NAME_OVERRIDES[str] ?? str.replace(/([A-Z])/g, '_$1').toLowerCase(); | |
| return ICON_NAME_OVERRIDES[str] ?? str.replace(/[A-Z]/g, (letter) => "_" + letter.toLowerCase()); |
There was a problem hiding this comment.
Fixed in 805fb5d. Aligned all three renderers to use the same regex pattern.
|
|
||
| function toMaterialSymbol(name: string): string { | ||
| if (ICON_NAME_OVERRIDES[name]) return ICON_NAME_OVERRIDES[name]; | ||
| return name.replace(/([A-Z])/g, "_$1").toLowerCase(); |
There was a problem hiding this comment.
Consider using a more targeted replacement function for consistency with other renderers and slightly better performance by avoiding a global lowercase call.
| return name.replace(/([A-Z])/g, "_$1").toLowerCase(); | |
| return name.replace(/[A-Z]/g, (letter) => "_" + letter.toLowerCase()); |
Only apply toMaterialSymbol when props.name is a string icon name, not when it's an SVG path object. Also align regex pattern across all three renderers for consistency.
|
This looks great - thank you!! Please just fix the |
|
Done, sorry for that!! |
2 participants
Description
Adds an icon name override map to all three v0.9 renderers (React, Angular, Lit) for A2UI spec icon names that don't match their Material Symbols ligatures after camelCase-to-snake_case conversion.
The four overrides:
playplay_arrowrewindfast_rewindfavoriteOfffavorite_borderstarOffstar_borderAdditionally, the Lit v0.9 renderer was missing camelCase-to-snake_case conversion entirely, so that is also added here.
Follow-up to #1031 as discussed in #1031 (comment).
Pre-launch Checklist