fix(react-renderer): fix icon rendering in v0.9 basic catalog #972
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -19,9 +19,44 @@ | |||||
| import {IconApi} from '@a2ui/web_core/v0_9/basic_catalog'; | ||||||
| import {getBaseLeafStyle} from '../utils'; | ||||||
|
|
||||||
| // 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); | ||||||
| } | ||||||
| } | ||||||
|
Comment on lines
+22
to
+33
Contributor
There was a problem hiding this comment. While automatically loading the font makes the component self-contained, injecting a 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 ICON_MAPPING: Record<string, string> = { | ||||||
| play: 'play_arrow', | ||||||
| rewind: 'fast_rewind', | ||||||
| favoriteOff: 'favorite', | ||||||
| starOff: 'star', | ||||||
| }; | ||||||
|
|
||||||
| const FILLED_ICONS = ['star', 'starHalf', 'favorite']; | ||||||
|
|
||||||
| export const Icon = createReactComponent(IconApi, ({props}) => { | ||||||
|
Contributor
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 Please add unit tests to cover the new logic, including:
References
|
||||||
| const rawIconName = | ||||||
| typeof props.name === 'string' ? props.name : (props.name as {path?: string})?.path; | ||||||
|
|
||||||
| // Use mapping if available, otherwise convert camelCase to snake_case | ||||||
| let iconName = ''; | ||||||
| if (rawIconName) { | ||||||
| if (ICON_MAPPING[rawIconName]) { | ||||||
| iconName = ICON_MAPPING[rawIconName]; | ||||||
| } else { | ||||||
| iconName = rawIconName.replace(/([A-Z])/gm, "_$1").toLowerCase(); | ||||||
|
Contributor
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., A small adjustment can make this more robust by removing the potential leading underscore.
Suggested change
|
||||||
| } | ||||||
| } | ||||||
|
|
||||||
| const isFilled = FILLED_ICONS.includes(rawIconName || ''); | ||||||
|
|
||||||
| const style: React.CSSProperties = { | ||||||
| ...getBaseLeafStyle(), | ||||||
| fontSize: '24px', | ||||||
|
|
@@ -30,6 +65,7 @@ | |||||
| display: 'inline-flex', | ||||||
| alignItems: 'center', | ||||||
| justifyContent: 'center', | ||||||
| fontVariationSettings: isFilled ? '"FILL" 1' : undefined, | ||||||
| }; | ||||||
|
|
||||||
| return ( | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.