feat(theme): add component icon mappings - #4647
Conversation
meta-cla
Bot
added
the
CLA Signed
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
| targets: [ | ||
| {className: 'astryx-selector', visualProps: ['size', 'status']}, | ||
| {className: 'astryx-selector-option'}, | ||
| {className: 'astryx-selector-selected-option-icon'}, |
There was a problem hiding this comment.
Should this leave a className?
There was a problem hiding this comment.
No — resolved. The new astryx-selector-selected-option-icon target is gone; the slot now rides main's existing astryx-selector-check target. Two independent axes on one element: componentIcons['selector-selected-option'] picks which glyph, astryx-selector-check styles how it looks. No new theme surface for this feature.
| </span> | ||
| {isSelected && <Icon icon="check" size="sm" color="accent" />} | ||
| {isSelected && selectedOptionIcon != null && ( | ||
| <span |
There was a problem hiding this comment.
Hmm should we handle most of this logic within Icon instead?
There was a problem hiding this comment.
Agreed, and that's where it lives now. The wrapper span is gone; the callsite is just:
<Icon icon="selector-selected-option" fallbackIcon="check" size="sm" color="accent" {...themeProps('selector-check')} />Icon does the slot resolution (useIcon), the fallback, and the render-nothing case when a theme maps the slot to null. Components just name the slot.
35bbb09 to
c6585c0
Compare
c6585c0 to
c740262
Compare
c740262 to
a6a370c
Compare
github-actions
Bot
added
the
needs:design-review
a6a370c to
769abee
Compare
769abee to
f98460b
Compare
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR Modified ComponentsIcon (@astryxdesign/core) · View in Storybook
Selector (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: 1 accessibility violation(s) found — 1 serious. Icon - 1 issue(s)
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
f98460b to
93ad54c
Compare
93ad54c to
61535c1
Compare
61535c1 to
4fb13b6
Compare
github-actions
Bot
removed
the
needs:design-review
| * Component icon slot mappings — maps component-specific purposes to global | ||
| * semantic icon names. Use `null` to intentionally render no icon. | ||
| */ | ||
| componentIcons?: ComponentIconMap; |
There was a problem hiding this comment.
This should be Partial, right?
There was a problem hiding this comment.
It already was, just hidden inside the alias — ComponentIconMap was declared as Partial<Record<ComponentIconSlotName, IconName | null>>. But that's exactly why the question is worth acting on: the field read as non-partial next to icons?: Partial<IconRegistry>.
Flipped it to match the sibling convention in 4423a81: ComponentIconMap is now the complete Record, and both theme fields declare Partial<ComponentIconMap>. Same runtime behavior, and partialness is now visible where it's used. Safe to reshape since the type isn't released yet.
| /** Icon registry */ | ||
| icons?: Partial<IconRegistry>; | ||
| /** Component icon slot mappings */ | ||
| componentIcons?: ComponentIconMap; |
There was a problem hiding this comment.
Done in the same commit — DefinedTheme.componentIcons is Partial<ComponentIconMap> too.
- Icon.doc.mjs: fallbackIcon's 'valid semantic names' list was missing chevronsLeft/chevronsRight (added on main), failing docPropLiterals. Also translates the docsZh copy, which was left in English. - ComponentIconMap is now the complete Record; theme fields declare Partial<ComponentIconMap>, matching icons?: Partial<IconRegistry>.
| : (input.icons ?? base?.icons); | ||
|
|
||
| // 6. Merge component icon mappings — input mappings override base mappings. | ||
| // `null` is an intentional override meaning “render no icon”. |
There was a problem hiding this comment.
I think we should skip null as an intentional override for now. It's often confused for "default" so if we do decide to add one later we should have a clear symbol.
There was a problem hiding this comment.
Agreed — dropped it in 96a764c.
A theme now maps a slot to another global icon name, or leaves it alone; there is no value that means "render nothing." ComponentIconMap values are IconName, and Icon's fallbackIcon / useIcon's fallback follow (no | null), so the "is this the default or is this off?" question can't be asked. The doc prose, the docsite icon-slots table, and the CLI component table all lose their "none" case too.
Left a comment on getComponentIconName recording why, so a future reader doesn't add it back casually: if suppression turns out to be a real need it should arrive as an explicit symbol.
And it isn't load-bearing for selection — an indicator draws its own empty state (an unchecked check indicator renders nothing) rather than relying on the slot to vanish.
`null` reads as "default" at least as often as it reads as "render nothing", so a theme can no longer suppress a component icon slot: it maps a slot to another global icon name, or leaves it alone. If suppression turns out to be a real need it should arrive as an explicit symbol that can't be confused with the default. The selection use case doesn't need it — an indicator draws its own empty state (an unchecked check indicator renders nothing) rather than relying on the icon slot to vanish. - ComponentIconMap values are IconName (no null); Icon's fallbackIcon and useIcon's fallback follow. - Doc prose, the docsite icon-slots table, and the CLI component table drop the "none" case.
| export function useIcon( | ||
| nameOrSlot: IconName | ComponentIconSlotName, | ||
| fallback?: IconName, | ||
| ): ReactNode { |
There was a problem hiding this comment.
It may be better to have useComponentIcon instead since the check for calling getComponentIcon is whether a fallback is defined but there's nothing guaranteeing a fallback is provided when component icon is provided in nameOrSlot.
There was a problem hiding this comment.
Good catch, and it was worse than the hook — done in d0e38d0.
Your read is right that nothing tied the two together. The overload did reject useIcon('selector-selected-option') (disjoint unions, no matching signature), but I probed the neighbor and found the real hole:
<Icon icon="selector-selected-option" /> // typechecked fineThat resolves through no fallback, so an unmapped slot renders nothing — a blank where the check should be, no error anywhere.
Two changes:
-
useComponentIcon(slot, fallback)as its own export, as you suggest;useIcon(name)goes back to a single signature. The fallback is now a required parameter of the function that needs it, instead of an optional second argument whose absence silently picked the lookup. No production callsite used the two-arg form, so the split is clean. -
IconPropsis a discriminated union — a global icon takesfallbackIcon?: never, a slot takesfallbackIcon: IconName(required). Same guarantee at the component, where the hole actually was.
Pinned with @ts-expect-error tests, since compile time is the only place these can be checked. Verified they bite by reverting the union — both directives went unused and the build failed.
A component icon slot is meaningless without the component's own default: an unmapped slot with no fallback resolves to nothing, so the icon silently disappears. Nothing enforced the pairing — `useIcon` chose its lookup from `arguments.length`, and `<Icon` accepted a slot with no `fallbackIcon` at all. - Split the overloaded hook: `useIcon(name)` for a global icon, `useComponentIcon(slot, fallback)` for a slot, with the fallback a required parameter of the one that needs it. - `IconProps` becomes a discriminated union: a global icon takes `fallbackIcon?: never`, a slot takes `fallbackIcon: IconName`. Pinned by @ts-expect-error tests, verified by reverting the union and confirming they fail.
Calling useIcon/useComponentIcon outside a component tripped rules-of-hooks and no-floating-promises (ReactNode includes a thenable in React 19's types). These only ever needed the compiler, so assert on the argument tuples instead.
A componentIcons key core does not expose is silent: resolution falls back to the component default, so a typo'd slot — or one carried by a built theme from a core version that has since renamed it — ships as a theme that simply does not apply. - `theme build` warns, following validateComponentOverrides: known slots are derived from the component docs (the same *.doc.mjs the docsite renders), not a second hardcoded registry, and validation is skipped entirely when docs are unavailable. - docIconSlots.test.ts pins the two directions: every slot declared in ComponentIconSlotMap is documented, and no doc names a slot core does not declare. Slot names are parsed from source, so a new slot fails the test until it is documented. This is also what keeps the build's derived known-set honest. - Docsite theming section states the rules a slot implies: appearance changes, semantics do not; slots take precedence over the icons registry; slot names are versioned API.
|
Closing this — we're deferring component icon mappings entirely, and taking the minimal API first. Why. Every motivating case for icon selection turns out to be stateful: the checkmark vs. radio dot in a selector option, the chevron on an expand toggle, the arrow vs. chevron for sort. Those are indicator families, and indicator replacement covers them without a slot map. The cases that genuinely need a static glyph swapped — the clear What replaces it. Deliberately leaving room. Slot maps stay additive: Worth keeping from this PR, as separate changes:
Thanks for the reviews — the Continuing in #4712, rescoped to indicators only. |
2 participants
Why
Some icon choices are component semantics rather than global glyph choices. Selector is a good example: its selected-option indicator defaults to
check, but a theme may want that same purpose to render a different registry icon or no icon at all.What
Adds themeable component icon mappings on top of the theme icon registry work:
componentIconstodefineTheme()/DefinedTheme, inherited throughextends.useIcon(slot, fallback)for component internals andgetComponentIcon()/getComponentIconName()for explicit-source resolution.selector-selected-optionslot.componentIconsinastryx theme buildoutput.Risk
The default Selector selected option remains a check icon. Themes can now remap or suppress that purpose through
componentIcons; this is additive unless a theme opts in.Testing
pnpm exec vitest run --project ui packages/core/src/Icon/globalIconRegistry.test.tsx packages/core/src/Selector/Selector.test.tsx packages/core/src/theme/defineTheme.test.ts --reporter=dotpnpm exec vitest run --project node packages/cli/clients/cli/lib/component-format.test.mjs packages/cli/api/theme/build/build.test.mjs --reporter=dotpnpm -F @astryxdesign/core typecheckpnpm -F @astryxdesign/core lintpnpm -F @astryxdesign/cli typecheck:authoringpnpm check:syncpnpm check:package-boundariespnpm check:changesetspnpm -F @astryxdesign/core build