feat(Spinner): make the ring geometry and colors themeable - #5214
Conversation
The size prop stays a fixed enum ('sm' | 'md' | 'lg' | 'xl'). A theme can now
redefine what each named size resolves to — its diameter and the rail (ring
stroke) width — via the --_spinner-diameter and --_spinner-rail-width custom
properties on the size-variant target, e.g.
spinner: { 'size:xl': { '--_spinner-diameter': '40px' } }.
Spinner is canvas-drawn, so CSS custom properties do not reach the paint
automatically. The draw effect reads the resolved vars back off the canvas via
getComputedStyle (falling back to the built-in SIZES constants), mirroring the
existing shade='inherit' read-back, and the wrapping box tracks the same vars
in CSS with calc(). Output is byte-identical when no theme overrides them.
meta-cla
Bot
added
the
CLA Signed
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
community
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR Modified ComponentsSpinner (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Visual Regression96 added · 0 removed. View the report To accept these exact frames: Added — After Added — After Added — After Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
cixzhang
left a comment
cixzhang
left a comment
There was a problem hiding this comment.
🔴 Blocking — the theming API this adds is rejected by astryx theme build, plus needs:code-review / needs:design-review.
Triage: new API surface (theming) · non-breaking by default · low blast radius → deep path · checks: T2, P1, X20, §derived-vars
Thanks — this is a real gap, and the care shows. I checked the "byte-identical" claim in Chromium against main's Storybook with the animation frozen, and it holds exactly: Sizes and WithLabel come out pixel-for-pixel the same. The size:xl key form is right, the specificity reasoning is right, the labeled-case cascade is right, EN + zh docs and a correct [feat]/patch changeset are all there, and folding the second getComputedStyle into canvasStyle (Spinner.tsx:217) is a nice cleanup.
Three things block. The first is a tier call — I've made it below, so this is a rework you can start on.
1. Use public vars, not private ones. --_ + private: true means "themes never set this directly" (Theming Infrastructure → Private vars), and validatePrivateVars in packages/cli/api/theme/build/build.mjs:1025 enforces it. Running the exact snippet from the description:
✗ Component "spinner" (size:xl) sets private var "--_spinner-diameter". Private vars (--_*)
are internal — use standard CSS properties instead. The pipeline expands them automatically.
✗ Component "spinner" (size:xl) sets private var "--_spinner-rail-width". …
2 private var error(s).
That snippet is also in the changeset and in Spinner.doc.mjs:47-48 / :103-104. (It still emits CSS after logging ✗, which is probably why the throwaway build read as a pass.)
Please go with --spinner-diameter / --spinner-rail-width — no underscore, private: false, set directly, the --button-press-scale precedent. The other route, derived vars (width → --_spinner-diameter, the progressbar-mark shape at derivedVarRegistry.ts:71-73, which is what you did for padding in #5181), is the right answer when the thing being themed genuinely is a CSS box property. Here it isn't: the ring is painted on a canvas, so width and borderWidth would name properties the element doesn't really have, and a theme author reading borderWidth on a spinner would reasonably expect a border. A public var says what it is. That does mean an entry in VARS_WITHOUT_DERIVED_MAPPING (derivedVarRegistry.test.ts:286) — fine, that list is public vars and self-set vars, which is exactly what these are.
2. Only px overrides survive. parseFloat(canvasStyle.getPropertyValue(…)) at Spinner.tsx:218-223 reads an unregistered custom property, so it gets the specified token and drops the unit — while the box at :73-74 goes through real calc(). Measured on this PR's Storybook, size:xl:
| theme sets | CSS box | drawn ring |
|---|---|---|
40px / 6px |
52px | 46px ✓ |
2.5rem / 0.375rem |
52px | 3.25px |
calc(2rem + 8px) |
52px | 36px (NaN → fallback) |
rail 0px |
28px | 36px canvas, clipped by overflow: hidden |
The last row is the || at :219/:222 reading 0 as absent. Registering the properties (@property { syntax: '<length>'; inherits: true }) makes the computed value a used px length and fixes the rem/em/calc rows in one move; Number.isFinite(x) ? x : default fixes the 0 row.
3. Box and ring only agree at mount. Deps are [shade, size, themeTokens] (Spinner.tsx:295), so a var change that isn't a size/shade/token change never redraws. Changing the var at runtime: box went 36px → 52px, canvas stayed 36px. Worth narrowing the description's "stay in sync … after a theme switch" to mount-time overrides — or observing the var.
Non-blocking:
- The six new tests all pass against
main'sSpinner.tsxunchanged (I swapped the file and ran vitest — 23/23). They assertthemePropsoutput that already existed and that four sizes hash to four distinct class strings. jsdom has no canvas and no layout, so nothing here can reach the actual contract; a Storybook check is the way, andneeds:design-reviewwants screenshots regardless. Spinner.tsx:69-73,:93-97,:211-217,:321-328restate the description in-file — a line or two each is plenty.- Dropping the inline
width/heightalso flips precedence: a consumer'sstyle={{width}}now wins whereframeSizeused to override it. Fine by me, worth a line in the changeset. - Pre-existing, not yours:
SpinnerProps.sizeJSDoc says xl is 36px, butSIZES.xl.diameteris 28 (36 is the frame), and it still says "three sizes". Happy to take that separately.
…public vars Reworks the theming surface after review. Three things were wrong with the first pass, and adding themeable color makes a fourth worth having. Public vars, not private ones. `--_*` means "the pipeline sets this, not you": `validatePrivateVars` reports every theme that writes one, so the API the docs and changeset advertised was one the build refuses. The four vars are now `--spinner-diameter`, `--spinner-rail-width`, `--spinner-color` and `--spinner-track-color`, set directly under a size- or shade-variant key — the `--button-focus-offset` / `--tree-list-indent` precedent. Derived vars are the other route and are wrong here: the ring is painted on a canvas, so `width` would name a box the element does not have, and mapping `color` would take the label's text color with it. Only px survived. `parseFloat` on an unregistered custom property reads the SPECIFIED value, so a themed `2.5rem` drew a 3.25px ring inside a 52px box and `calc()` drew the fallback. The two geometry vars are registered as `<length>` (CSS.registerProperty, guarded and idempotent), which makes their computed value an absolute px length: rem, em and calc all arrive resolved. A rail of `0` is now honoured too — it read as "absent" through `||` and fell back to the default, drawing a 36px ring in a 28px box that `overflow: hidden` then clipped. Box and ring only agreed at mount. The draw ran on [shade, size, themeTokens], so a var change no dependency could see left the box resized and the ring where it was. The box is sized from the same vars, so its resize IS the signal: a ResizeObserver on it redraws, and a cheap key check keeps that to one draw on mount. This also covers a media query swapping the var, and a root font-size change moving a rem. Color, the new part, goes through a real `color` property rather than a custom property read back raw — the arc on the canvas, the track on the box it sits in. That indirection is what resolves `var()`, `color-mix()` and the `currentColor` the inherit shade is built on into something canvas can stroke; a registered `<color>` custom property computes `currentColor` to the literal string. It generalizes the read-back the inherit shade already used, and the per-shade tokens move from JS into the CSS fallbacks, leaving one code path. The onMedia track's `4D` hex suffix becomes globalAlpha 77/255 — the same composite, no longer requiring the token to be hex. Verified in Chromium against builds of main, of the reviewed commit, and of this one: the 16 size x shade defaults are pixel-identical to main (canvas bitmaps compared as data URLs), every themed spelling above now draws a ring that matches its box, and each of the five color cases changes the painted output where it was previously inert. The six previous tests passed against an unmodified Spinner. These fail against main: 10 of them, covering which element declares each var, what each size and shade resolves to, and that a themed value reaches the target. build.public-component-vars.test.mjs runs the real builder over the documented snippet — the check that would have caught the private-var problem. It asserts on the receipt's warnings, not a rejection: the build reports a private var and then emits its CSS anyway, which is why a throwaway build read as a pass. Also fixes the doc drift the review noted, in the lines this touches: `size` omitted xl and called 28px "36px" (that is the box), and the component was described as having three sizes and two shades.
freddymeta
changed the title
…nput `Record<string, unknown>` does not satisfy ComponentStyleMap, so `pnpm -F @astryxdesign/core typecheck` — which covers tests, and which I had run before writing them rather than after — failed in CI.
Found auditing the 23 internal Spinner call sites. Several components paint a spinner inside a fixed-size control and are flex containers: a Switch thumb is 14×14 at the smallest size — exactly the default box, zero headroom — and a CheckboxInput indicator is 20×20. The box carries no `flex-shrink`, so it defaults to 1 and the parent is free to compress it. The canvas is not laid out by that parent, though: it keeps drawing at whatever the vars resolve to. Today the two never disagree, because the diameter cannot be themed. Once it can, they do — measured in Chromium with `--spinner-diameter: 24px`: Switch sm (off) ring 32px in a 14px box ring painted outside its own box Switch md (on) ring 32px in a 20px box " CheckboxInput sm ring 32px in an 18px box " and `overflow: hidden` on the box then clips the ring it is meant to contain. With `flex-shrink: 0` all six report 32/32: the box and the ring stay the same measurement, so a themed size that does not fit its host overflows visibly at the host rather than being silently cut in half. Defaults are untouched — all 26 measured size × shade cases are identical before and after, and the whole suite passes.
The four public vars were documented as themeable, and the theme pipeline
emitted the right rule for them, but a themed diameter or rail width had
no effect in a browser: the default rings and the themed rings measured
identically (14/20/24/36px in both rows of the new Storybook story).
StyleX assigns custom-property declarations priority 0 and emits priority-0
rules OUTSIDE its cascade layers (babel-plugin: `useLayers && pri > 0`),
while a theme's component overrides are injected into `@layer astryx-theme`.
Unlayered declarations beat every layer regardless of specificity, so
`sizeStyles` declaring `--spinner-diameter: 10px` shadowed the theme's
`.astryx-spinner.sm { --spinner-diameter: 1rem }` permanently. The two color
vars were never affected, because they are read with the shade's token as a
`var()` fallback and never declared — which is exactly the shape the geometry
now uses too.
Each size now resolves into a private `--_spinner-ring-*` pair:
--_spinner-ring-diameter: var(--spinner-diameter, 10px)
so the theme's declaration is the only declaration of the public var, and the
size's default applies precisely when there isn't one. The registration moves
to the private pair with it — registering the public vars would give them an
initial value, and a registered property is never guaranteed-invalid, so the
`var(..., default)` fallback would stop firing and every unthemed spinner
would draw at 0px.
Measured in Chromium against the built Storybook:
default sm 14 md 20 lg 24 xl 36 (unchanged)
themed sm 20 md 32 lg 42 xl 52 (was 14/20/24/36)
The themed values are `rem` and `calc(2rem + 8px)`, so this also shows the
registered length resolving rather than being read as text. A rail of `0`
draws a 28px ring in a 28px box, unclipped. Sizes, shades and the labelled
layout are pixel-identical to before.
Also here, from the review notes on the first version:
- Deleted the computed-style assertions from Spinner.test.tsx. They passed
against an unmodified Spinner.tsx, and per the rubric's V4 they were slop —
worse, two of them asserted the very declaration that made the feature
inert. What's left asserts the theme routing (a size key must land on
`.astryx-spinner.xl`, not on the bare target), which is a real contract.
- Extracted the geometry fallback into `resolveRingGeometry`, with tests for
the deliberate asymmetry: diameter 0 falls back, rail 0 is honoured.
- Made the CLI's public-var test doc-driven — it now scans every `*.doc.mjs`
and builds a theme naming every non-private, non-derived var, so a new
component gets the check for free. Filtered on the `private` flag rather
than the `--_` prefix, so a mis-prefixed var is caught instead of skipped.
- Storybook: `ThemedGeometry` and `ThemedColor` stories, and the `shade`
argTypes had gone stale (missing `subtle` and `inherit`).
cixzhang
left a comment
cixzhang
left a comment
There was a problem hiding this comment.
Thanks — our module-scope ask was fixed. Ruby's two are now in too: the name matches both circles, box sizing keeps its old precedence, and defaults are pixel-identical to main.
[Reviewed by Robohands]
github-actions
Bot
removed
the
needs:code-review
|
@rubyycheung re-requesting — both of your asks are in
cixzhang has approved since. Yours is the one still open. |
cixzhang
left a comment
cixzhang
left a comment
There was a problem hiding this comment.
Re-approving after the mechanical main refresh; the reviewed change is unchanged.\n\n[Reviewed by Robohands]
cixzhang
left a comment
cixzhang
left a comment
There was a problem hiding this comment.
Re-approving after the mechanical main refresh; the reviewed change is unchanged.
[Reviewed by Robohands]
cixzhang
left a comment
cixzhang
left a comment
There was a problem hiding this comment.
Re-approving after the mechanical main refresh; the reviewed change is unchanged.
[Reviewed by Robohands]
|
/accept-visual 33045918232/1 These 12 frames add Spinner coverage; no existing pixels changed, and sensor-backed review verified unchanged defaults plus the intentional themed geometry. |
|
Visual acceptance refused: only a repository maintainer may accept stable visual changes. |
|
/accept-visual |
|
Visual acceptance refused: copy the exact |
|
/accept-visual 33045918232/1 These 12 frames add Spinner coverage; no existing pixels changed, and sensor-backed review verified unchanged defaults plus the intentional themed geometry. |
|
Visual acceptance refused: only a repository maintainer may accept stable visual changes. |
|
/accept-visual 33045918232/1 These 12 frames add Spinner coverage; no existing pixels changed, and sensor-backed review verified unchanged defaults plus the intentional themed geometry. |
github-actions
Bot
added
the
visual-approved
|
Visual changes accepted for |
Both requested contract fixes are present at the current head: the variable is now --spinner-stroke-width, and existing box-sizing precedence is restored. The refreshed head was re-reviewed and approved.
3 participants
Summary
Makes the Spinner's ring themeable — its per-size geometry and its two colors — without changing either prop's type, and without changing anything a consumer can observe today.
sizeandshadestay fixed enums; what each named value resolves to becomes a theme's to set.Four public custom properties on the
spinnertarget:--spinner-diameter--spinner-stroke-width--spinner-color--spinner-track-colorAny length and any color notation works:
rem,emandcalc()resolve into the radius and stroke the ring is drawn with, and colors acceptvar(),color-mix()andcurrentColor.How the SVG honours the vars
randstroke-widthare real CSS properties on an SVG shape, and a CSS declaration outranks the presentation attribute of the same name. So the attributes stay as the size's defaults — what a server render and a no-CSS render draw — and the cascade takes over the moment a theme has a value. The colors ridestrokedirectly. The dash pattern is composed from the same resolved diameter (diameter x pi x arc-fraction), so a themed ring keeps the same fraction of arc rather than the same absolute dash: every themed row below measures 135.000°.The box is sized from the same two vars (
calc(diameter + 2 x stroke)), so the box and the ring are one measurement by construction rather than by a listener keeping them in step — and that size is applied as an inlinewidth/heightwritten after the caller'sstyle, which is exactly where the number used to go.Why public vars, and not derived or private ones
--_*means "the pipeline sets this, not you" —validatePrivateVarsreports every theme that writes one — so the first revision of this PR advertised an API the build refuses. These are public, set directly, following--button-focus-offsetand--tree-list-indent.Derived vars are the other route, and they are right when the thing being themed genuinely is a CSS property of the element (#5181's padding). Mapping
widthhere would name the box rather than the ring, and mappingcolorwould take the label's text color with it. A public var says what it is.sizeStylesandshadeStylesdeclare the four vars on the element carrying thespinnertheme target — the span, or the wrapper when a label moves the target there, never both. The box and the ring read them through a still-private registered<length>pair, so a themed stroke of0stays a zero-width stroke instead of poisoningcalc(28px + 0 * 2). The four public vars stay unregistered on purpose: a registered property has aninitial-value, so every element reports a value for it, andtheme-var-reachability.jsfinds a var's declaring element by exactly that test.Review changes in the latest revision
--spinner-rail-widthis now--spinner-stroke-width(@rubyycheung). The old name reads as the stationary track — and--spinner-track-colorbeside it made that the natural reading — while it is the stroke width of both circles, so a themed0takes the moving arc with it. Deliberately not split into two widths: nothing has asked to set the arc and the track independently, and a var per circle is a wider public contract than the demand for it. The private resolved var follows the name, and the doc entry in both locales now leads with the one-width-drives-both fact.The sizing precedence is unchanged, and the
[breaking]changeset is gone (@rubyycheung). Sizing the box from the vars had moved the declaration out of the inline style and into a rule, which would have handed a caller'sstyle={{width}}a precedence over the box that it has never had. Instead the size is composed into an unregistered--_spinner-box-sizeand applied inline after the caller'sstyle, with the size's own frame as thevar()fallback for a render where no stylesheet has declared it. So there is no behaviour change left to separate out or to describe:main<Spinner size="xl"><Spinner size="xl" style={{width: 999, height: 999}}><Spinner size="xl" style={{opacity: .5, margin: 7}}><Spinner size="xl" label="…" style={{width: 999}}>pathLengthis gone — found while re-measuring, and it was mine. Normalizing the arc to the default circumference rescales the dash against the path length the UA measures on its own approximation of the circle: 87.398 against the 87.965 ofpi x 28. That shortened the default arc by 0.64% and moved the cap by half a pixel — 191 changed pixels on anxlspinner againstmain, in a PR whose whole claim is that defaults do not move. Composing the dash from the resolved diameter is exact at the default and scales identically when a theme moves it.Registration is at module scope (@cixzhang) — pushed in
42ae22fc, about half an hour after the review that asked for it, so that block was written against the previous head. Registering an inherited property with aninitial-valueinvalidates style for the whole document, and a spinner mounts onto a page that has already rendered.What a stroke width of
0does — asked in reviewIt paints nothing, and that is what it should do. One
stroke-widthdrives both circles, so zeroing it is a zero-width stroke on each:box 28×28, r 14px, stroke-width 0px— an empty box that still announces "Loading". That is whatstroke-width: 0means everywhere else in CSS, and the case people actually want — an arc with no track behind it — is--spinner-track-color: transparent, independent of geometry. The doc entry says so, and the story that promised a trackless arc is a 1px hairline over a transparent track, which is what it was trying to show.Verified in Chromium 149
Defaults are untouched. Every size × every shade × the labelled case × an inline-in-text case, at four frozen rotation angles: 72 of 72 element screenshots byte-identical between a build of
origin/mainand a build of this branch, with box geometry and position identical.rstroke-widthA theme moves them, through a built Storybook so the values travel the real
@scope+@layer astryx-themepath:rstroke-width1rem/0.125rem1.5rem/0.25rem2rem/0.3125remcalc(2rem + 8px)/0.375rem1pxColors: a themed arc and wash resolve (
rgb(0,69,140)overrgb(196,221,251)), andshade:subtlewithtransparentgivesrgba(0,0,0,0)on that spinner only.Reachability guard, against a full production build:
Split out of this PR, at review request
main. Its cause turned out to be the box's leftoveroverflow: hidden, which zeroes a flex item's automatic minimum size; the PR has the measurements.[breaking]changeset is deleted.Test plan
pnpm build,pnpm lint:strict(0 errors; the same 80 warningsorigin/mainreports),pnpm check:repo,pnpm -F @astryxdesign/core typecheck— all clean.pnpm exec vitest run packages/core/src/Spinner packages/core/src/theme packages/cli/api/theme: 28 files / 927 tests pass.size:xlkey has to land on.astryx-spinner.xl, asserted on the whole emitted rule), the authored dash staying absolute with nopathLength, the registration timing, and the box-sizing precedence (style={{width: 999}}leaves the composed value in place, other style properties pass through). The cascade itself has no jsdom equivalent — the Chromium numbers above are its evidence.build.public-component-vars.test.mjsis doc-driven: it scans every*.doc.mjs, builds one theme naming every non-private, non-derived var, and asserts the builder reports no private-var warning, so a new component earns the check for free.@freddymeta