fix(react-renderer): fix icon rendering in v0.9 basic catalog#972
fix(react-renderer): fix icon rendering in v0.9 basic catalog#972jacobsimionato wants to merge 1 commit intogoogle:mainfrom
Conversation
![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 programmatic loading of the Material Symbols font, adds icon name mapping, and converts camelCase names to snake_case for the Icon component, also applying fill settings for certain icons. Feedback suggests reconsidering the programmatic font loading for better control by the application, adding unit tests for the new icon logic as per the style guide, and fixing a potential bug in the camelCase to snake_case conversion regex that could lead to leading underscores.
| // Programmatically load the material symbols font if the application hasn't already. | ||
| if (typeof document !== 'undefined') { | ||
| const isLoaded = Array.from(document.querySelectorAll('link')).some( | ||
| (link) => link.href.includes('Material+Symbols+Outlined') | ||
| ); | ||
| if (!isLoaded) { | ||
| const link = document.createElement('link'); | ||
| link.rel = 'stylesheet'; | ||
| link.href = 'https://fonts.googleapis.com/css2?family=Material+Symbols+Outlined:opsz,wght,FILL,GRAD@24,400,0,1'; | ||
| document.head.appendChild(link); | ||
| } | ||
| } |
There was a problem hiding this comment.
While automatically loading the font makes the component self-contained, injecting a <link> tag as a module-level side effect is generally discouraged in React. It can have unintended consequences in different environments (like SSR), can be hard to test, and takes control away from the application developer who might want to manage font loading differently (e.g., self-hosting).
A more robust approach for a component library is to remove this logic and instead document that the consuming application is responsible for including the 'Material Symbols Outlined' font. This gives developers more control and makes the component's behavior more predictable.
|
|
||
| const FILLED_ICONS = ['star', 'starHalf', 'favorite']; | ||
|
|
||
| export const Icon = createReactComponent(IconApi, ({props}) => { |
There was a problem hiding this comment.
The changes in this file introduce new logic for icon name mapping and font loading, but there are no corresponding tests to verify this new functionality for the v0.9 Icon component. According to the repository's style guide, code changes should be accompanied by tests.
Please add unit tests to cover the new logic, including:
- Correct mapping of icon names from
ICON_MAPPING. - Correct conversion of camelCase names to snake_case.
- Correct application of the
fontVariationSettingsfor filled icons.
References
- Code changes should have tests. (link)
| if (ICON_MAPPING[rawIconName]) { | ||
| iconName = ICON_MAPPING[rawIconName]; | ||
| } else { | ||
| iconName = rawIconName.replace(/([A-Z])/gm, "_$1").toLowerCase(); |
There was a problem hiding this comment.
The regular expression used to convert camelCase to snake_case can produce a leading underscore for PascalCase icon names (e.g., MyIcon becomes _my_icon). While the current icon set might not use PascalCase, this could be a source of bugs for custom icons.
A small adjustment can make this more robust by removing the potential leading underscore.
| iconName = rawIconName.replace(/([A-Z])/gm, "_$1").toLowerCase(); | |
| iconName = rawIconName.replace(/([A-Z])/gm, "_$1").toLowerCase().replace(/^_/, ''); |
ava-cassiopeia
left a comment
ava-cassiopeia
left a comment
There was a problem hiding this comment.
Is this matching behavior seen in other, competing frameworks?
I ask because unless we're trying to match functionality, I'd actually strongly recommend against this.
This creates an opinionated link to Material Symbols, when downstream users may want to use a different icon library. We probably ought to provide a way to override the font library used (even the MatIcon, which is more Material-opinionated, provides an option for this).
I'm also wary of importing a whole icon library due to weight, as most icon libraries offer incremental loading and can be quite heavy if imported in full. Then again, I suppose the AI could produce any icon name...and speaking of, do we tell the agent what icons are available to it..? That seems like something we need if we're gonna choose the library here.
| // Programmatically load the material symbols font if the application hasn't already. | ||
| if (typeof document !== 'undefined') { | ||
| const isLoaded = Array.from(document.querySelectorAll('link')).some( | ||
| (link) => link.href.includes('Material+Symbols+Outlined') |
There was a problem hiding this comment.
This will in some edge cases end up burning you. Material Symbols offers dynamic icon loading from their CDN, meaning that users can provide Material Symbols URLs that look like:
<link rel="stylesheet" href="https://fonts.googleapis.com/css2?family=Material+Symbols+Outlined:opsz,wght,FILL,GRAD@20..48,100..700,0..1,-50..200&icon_names=close" />Note the icon_names=close, which will cause that to only load the close icon and nothing else, saving significantly on weight.
This code will match the above href, even though it doesn't pull in the full library, which I guess we probably need for the sake of the AI choosing any icon? More on that in my other comment.
2 participants
Description of Changes
Icon.tsxthat programmatically injects the Google Fonts stylesheet for "Material Symbols Outlined" if it's not already detected in the document. This makes theIconcomponent self-contained.ICON_MAPPINGdictionary to bridge the gap between A2UI specification names (e.g.,play,rewind,starOff) and the actual ligatures required by Material Symbols (e.g.,play_arrow,fast_rewind,starwith outlined setting).FILLvariation axis. This allows icons likestarandfavoriteto correctly render as filled when requested, while their "off" variants render as outlined.Rationale
The v0.9 React renderer's
Iconcomponent was previously just rendering the raw icon name string. This failed for two reasons:play) don't have exact string matches in the Material Symbols font family (which usesplay_arrow).These changes ensure that icons rendered via the React v0.9 catalog match the visual behavior of the v0.8 Lit/Angular renderers and follow the design requirements of the Material Symbols variable font.
Testing/Running Instructions
18_track-list.json).playicon and other system icons (likestar,favorite) render correctly as glyphs rather than raw text.<head>of the document upon the firstIconcomponent mount.Fixes #946