Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size SummaryNo component packages changed. Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This needs an owner decision before it can be approved. The Spinner touch glyph and the draft spec:AST-094 it rests on (FR1–FR23, DEC-1 and DEC-2) have no current spec authority, and the PR itself marks them pending owner review. An engineering owner records the decision by commenting /approve-spec 51e5504e01c6445909380628ea6c6f6d609cc139 on this pull request (the author can do this as an engineering owner), or the glyph waits until AST-094 is current. Whatever that decision is:
- Land #8045, #8052, #8053–#8056, #8062, #8067 and #8177 first, or rebase onto them, so this diff is only the Spinner commit (1,068 lines instead of 15,106).
- Fix the references to files that do not exist (
motion/MEASUREMENTS.mdlines 124 and 254,motion/uikit/gestures.tslines 7 and 30, thesources.test.tsmentions in threeuikit/*.tsheaders, andSpinner.test.tsxline 522), then re-runlint, which ran out of memory on this head.
[Automated review]
| where a switch outside any scroll view is needed. | ||
|
|
||
| Done (iOS 26.2 simulator, Oct 23 2026; in the switch's own table, | ||
| `uikit/switch.ts`): M-SW02, M-SW03, M-SW04, M-SW05, M-SW06, M-SW09 (UISwitch |
There was a problem hiding this comment.
uikit/switch.ts does not exist at this head (nor does uikit/contextMenu.ts, named at line 254), so these measurements are recorded as done in tables the repository does not have. Point at the file that holds them, or drop the claim.
| * tap-to-click, long press, double tap and the start of a page scroll, so | ||
| * many guesses can be settled by measuring Safari itself. | ||
| * | ||
| * SYNC: When modified, update packages/core/src/gestures/README.md (the |
There was a problem hiding this comment.
packages/core/src/gestures/README.md (and the ../../gestures system named at line 7) does not exist at this head. Point the SYNC note at a file that exists.
| // `transform`, a property the browser animates without the main thread. | ||
| // A `stroke-dashoffset` (or any other paint) animation would repaint the arc | ||
| // on the main thread every frame and stop for as long as a script runs — | ||
| // the browser proof (Spinner.interaction.browser.spec.ts) holds the frames. |
There was a problem hiding this comment.
The browser proof is __tests__/SpinnerMotion.a11y.chromium.spec.ts; there is no Spinner.interaction.browser.spec.ts.
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: the Spinner touch glyph and the draft spec:AST-094 it rests on (FR1–FR23, DEC-1 and DEC-2) have no current spec authority, and the PR itself marks them pending owner review. An engineering owner records that decision by commenting /approve-spec 098463d016d185d39e6ecb20a60aa89c66f8a3f9 on this pull request (the author can do this as an engineering owner), or the glyph waits until AST-094 is current on main. Whatever the decision, these changes are still needed:
- Land #8045, #8052, #8053–#8056, #8062, #8067 and #8177 first, or rebase onto them, so this diff is only the Spinner commits (1,120 authored lines instead of 15,158); rebasing onto
mainalso clearspr-a11y, which fails on a test stepmainadded for a file this branch does not have. - Keep each Spinner inside its box when no stylesheet applies: at this head it renders the track, the arc and eight black spokes side by side, 140–348 px wide; a
hiddenattribute on the glyphspan(Spinner.tsxline 735) fixes the spokes. - Fix the references to files that do not exist (
motion/MEASUREMENTS.mdlines 124 and 254,motion/uikit/gestures.tslines 7 and 30, thesources.test.tsmentions in threeuikit/*.tsheaders, andSpinner.test.tsxline 522).
[Automated review]
| /> | ||
| </svg> | ||
| </span> | ||
| <span aria-hidden="true" {...stylex.props(styles.glyph)}> |
There was a problem hiding this comment.
Without a stylesheet this glyph is visible: each Spinner renders its track, its arc and these eight spokes side by side (black, since the <rect> has no fill), 140–348 px wide instead of its 14–36 px box. A hidden attribute here keeps the spokes out of the no-CSS render; the StyleX display: block under (pointer: coarse) still outranks the browser's [hidden] rule.
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: the iOS Spinner glyph and the draft spec:AST-094 it rests on have no current spec authority, and the pull request marks them pending owner review. An engineering owner (the author is one) records the decision by commenting /approve-spec 246d53016c9a414480b2c9c98500f32de8646d96, or the glyph waits until AST-094 and a Spinner record are current on main; whichever way it goes, these are still needed:
- Land #8045, #8052, #8053–#8056, #8062, #8067, #8177, #8257 and #8258 first, or rebase onto them, so this diff carries only the Spinner work.
- Scale the spoke delays in
Spinner.tsx(lines 547–554) with the cycle variable on line 414: withprogress.activityIndicator.cycleMsat 1,600 ms the head sits at 9 o'clock for 800 ms and then races round. - Add a Changeset naming
@astryxdesign/clifor the theme docs and template, and regeneratepackages/cli/test/__golden__/no-icon-set/docs-topics.json, whose motion skeleton no longer matches;lintandtest-nodefail on them. - Put
hiddenon the glyphspan(line 758) so a page without CSS does not show ten graphics per Spinner, and either amendspec:AST-012FR2 or drop the adaptationplatformcondition. - Point the references at files that exist (
MEASUREMENTS.md124 and 254,uikit/gestures.ts7 and 30,sources.test.tsin threeuikit/*.tsheaders,Spinner.test.tsx541).
[Automated review]
| s0: {animationDelay: '-600ms'}, | ||
| s1: {animationDelay: '-500ms'}, | ||
| s2: {animationDelay: '-400ms'}, | ||
| s3: {animationDelay: '-300ms'}, | ||
| s4: {animationDelay: '-200ms'}, | ||
| s5: {animationDelay: '-100ms'}, | ||
| s6: {animationDelay: '0ms'}, | ||
| s7: {animationDelay: '-700ms'}, |
There was a problem hiding this comment.
These delays are one-eighth of 800 ms, but the duration reads --motion-progress-activity-indicator-cycle-ms, which a theme may retune. With the cycle at 1,600 ms the head sits on 9 o'clock for 800 ms, all spokes dim for about 100 ms, then the head sweeps the rest in 700 ms. Scale each delay with the same variable, e.g. calc(var(--motion-progress-activity-indicator-cycle-ms, 800ms) * -0.75) for s0 … * -0.875 for s7, and add a test with a non-default cycle.
| /> | ||
| </svg> | ||
| </span> | ||
| <span aria-hidden="true" {...stylex.props(styles.glyph)}> |
There was a problem hiding this comment.
Without a stylesheet these eight spoke graphics render beside the track and the arc, so each Spinner shows ten graphics, 140–348 px wide. A hidden attribute on this span keeps the no-stylesheet fallback to the ring; the gated display: block still wins when CSS loads.
| * is everything else. Pair `ios` with `pointer: 'coarse'` for a touch | ||
| * profile, such as touch motion tuned per platform. | ||
| */ | ||
| platform?: 'ios' | 'other'; |
There was a problem hiding this comment.
platform adds a fifth condition field, while spec:AST-012 FR2 closes the when vocabulary at width, pointer, contrast and motion and says new fields need explicit additions to it. Amend AST-012 (FR2, platform support, verification) alongside this, or drop the field.
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| '@astryxdesign/core': patch | |||
There was a problem hiding this comment.
The CLI's motion and author-a-theme docs and theme.template.ts change too, and lint fails because no Changeset names @astryxdesign/cli. Add one for them.
There was a problem hiding this comment.
An engineering owner needs to decide on the spec before this can be approved: the iOS activity-indicator glyph rests on the draft AST-094 and on choices the description itself lists as "Proposed; pending owner review", and there is no Spinner spec for them to live in. Approval needs a maintainer to provide or identify the governing spec for the glyph (its sizes, colour, Reduce Motion behaviour and theme-retunable cycle) and to settle the @supports (-webkit-touch-callout: none) gate, which current AST-013 FR6 rules out; any engineering owner, the author included, can record that decision by commenting /approve-spec <full head SHA>. Whatever that decision is, these still need changes:
- Land the stacked pull requests first, or rebase onto them, so this one carries only the Spinner commits.
- Scale the spoke delays with
--motion-progress-activity-indicator-cycle-ms(Spinner.tsx:547–554), because with any cycle but 800 ms the glyph stalls on one spoke. - Amend AST-012 FR2 for the new
platformadaptation condition, or drop the condition. - Show on a real iOS Safari device that the rotating ring does not bring back the #6253 wobble that #6311 fixed.
- Keep the no-CSS render to one graphic (
hiddenon the glyph atSpinner.tsx:758) and fix the references to files and to an AST-090 record that do not exist.
[Automated review]
| * | ||
| * This replaces a `stroke-dashoffset` animation (#6311), which repainted the | ||
| * arc on the main thread every frame and froze for as long as a script ran. | ||
| * That animation had in turn replaced a rotating `<svg>` reported to wobble |
There was a problem hiding this comment.
This brings back rotation, the approach #6311 moved away from: its changeset says willChange: 'transform' on the rotating svg did not stop WebKit re-rasterizing the rounded cap, and the dash fix was confirmed on a real iOS Safari device. The ring still shows for WebKit with a fine pointer (an iPad with a trackpad), and nothing here shows the wobble stays fixed; a real-device iOS Safari check of the turning ring, or keeping the dash animation until one exists, would close it.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
FR7 (and FR8, via line 235) rests on amendments to a draft spec:AST-090, but neither this branch nor main has an AST-090 record, so a reader cannot check what is being amended. Point it at the record that exists, or add AST-090 first.
…ers change at once (#8272) ## Exact decision needed What a person who prefers reduced motion sees when a core component animates a change, on touch and on other pointers, and whether a component may keep its motion. ## Existing authority searched - `design:motion` DR5 (draft): every animated transition has an immediate or minimally moving form. Draft, so not authority. - `spec:AST-013` FR13: reduced-motion behavior is unchanged by server-rendered entrances; it defines none. - `spec:AST-012` FR2: `motion: reduce` and `pointer: coarse/fine` are the adaptation conditions. - No current web record defines component behavior under reduced motion. - Search terms: reduced motion, prefers-reduced-motion, cross-fade, Reduce Motion, motion DR5, spring, travel. Overlapping open pull requests: #7367 (draft `design:motion` DEC-2, hover entrances; no reduced-motion rule), #8214 (layer fade under a finger with Reduce Motion; conforms once its fade is 200 ms or less), #8052 (AST-094 FR8 carries the matching touch rule), #8211, #8239, #8253 (touch indicator and placeholder work; continuous indicators are outside this record). ## Decision New current record `design:reduced-motion`, matching iOS Reduce Motion with Prefer Cross-Fade: - DR1: under `prefers-reduced-motion: reduce`, a core component's entrances, exits, presentation switches, and state changes never move, scale, resize, or spring. - DR2: on a coarse pointer, they cross-fade: opacity only, 200 ms or less. - DR3: on any other pointer, they complete at once. - DR4: no core component, prop, or theme value keeps motion under reduced motion. `spec:AST-057` FR5 names that a swiped row's slide out, spring back, and hold-out follow DR2 under reduced motion. The draft `design:motion` keeps its other rules unapproved; its DR5 points to the new record. A separate record keeps the promotion to this one rule, as `docs/design/README.md` advises for a different approval lifecycle. ## Rejected alternatives - A per-component setting that keeps full motion under reduced motion. ## Claim scope and non-goals Only transitions under reduced motion. Motion without the preference, durations and easing, tokens, continuous indicators such as a spinner, and content the person drags with their own finger are outside this record. ## Evidence and implementation relationship Layers that open under a finger with Reduce Motion appear in one frame today, where iOS cross-fades. The open fix #8214 makes them fade under a coarse pointer and stay immediate under a mouse, which conforms to DR2 and DR3 once its fade is 200 ms or less. The open draft #7367 (`design:motion` DEC-2, hover-summoned layers) does not touch reduced motion. The AST-094 revision in #8052 carries the matching touch rule. No implementation is included. ## Writing rubric results - R1 narration: none. - R2 research in the record: none. - R3 evidence naming a change: none; verification names browser states. - R4 rejected alternatives over one line or outside a decision: one, inside DEC-1, one line. - R5 pull request or issue references remaining: none. - R6 ruled open questions still listing options: none. ## Scope - [x] This PR contains specification records only. - [x] It records observable behavior and preserves equivalent internal implementations. - [x] Any named internal mechanism is intentionally public, names its dependent caller/system, and explains why an equivalent implementation would not satisfy the contract. - [x] Verification states the evidence layer and failure signal without prescribing CI job/workflow topology. - [x] It records one intentional durable decision, not a rescue for a separable tagalong. - [x] The canonical knowledge owner, affected current claims, and overlapping open work are named; any ownership collision is resolved or called out for human decision. - [x] Adjacent API, visual, accessibility, composition, and implementation facts remain outside scope unless this exact decision depends on them. - [x] Public text and artifacts contain no internal Meta context. ## Validation - `pnpm check:knowledge` (CI lint) - Prettier - `git diff --check`
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
|
Agreed: one implementation.
|
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the spec: the iOS activity-indicator glyph rests on the draft AST-094 and on choices the description lists as "Proposed; pending owner review" (spokes in place of the ring, the sizes, UIKit's gray, and an unchanged pace under Reduce Motion), and there is no Spinner spec for them to live in. Approval needs a maintainer to provide or identify the governing spec for the glyph, and any engineering owner can record that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Whatever that decision is, these still need changes:
- Land the stacked pull requests first, or rebase onto them, so this one carries only the Spinner commits.
- Resolve touch through the shared resolver instead of the
-webkit-touch-calloutengine check (AST-013 FR6), as agreed in the thread. - Use the theme's compact definition instead of the 744 px query (AST-012 FR9), and drop
when.platformor amend AST-012 FR2 first. - Show the rotor on a real iOS Safari device, validate theme motion role ids against their tables, and fix the references to files and to an AST-090 record that do not exist.
[Automated review]
|
Done in 1bc4bf7: the glyph, its gray and the ring's hand-off key on |
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the spec: the Spinner touch glyph (eight gray spokes on every coarse pointer, a gray default shade, a pace that ignores reduced motion and a theme-retunable cycle) and the new theme motion roles have no governing spec on main, and the description lists them as "Proposed; pending owner review". A maintainer needs to provide or identify that governing spec, and to say whether the glyph's always-mounted spokes may stand under the minimal-DOM invariant of the component style authoring record; any engineering owner, the author included, records the decision by commenting /approve-spec 6f263ad3439d616ba8531d5eef2b7ef79f4e1557 on this pull request. Whatever that decision is, these still need changes:
- Land #8076 and #8177 first, or rebase onto them, so this pull request carries only the Spinner commits (31,723 authored lines today).
- Keep the glyph visible under forced colours: the spokes paint with a background that a contrast theme replaces, so nothing shows.
- Bring the velocity tracker in line with the one-release-filter rule, where a finger held still before lifting reads 0, or leave it out with the stack.
- Describe the glyph in
Spinner.doc.mjsas appearing under any coarse pointer, correct the three release notes that misdescribe it, and show the rotor in WebKit with a fine pointer.
[Automated review]
| insetInlineStart: 0, | ||
| width: `var(${BOX_SIZE})`, | ||
| height: `var(${BOX_SIZE})`, | ||
| backgroundColor: 'var(--spinner-color)', |
There was a problem hiding this comment.
Under forced colours this background is replaced with Canvas (forced-color-adjust stays auto), so every spoke computes to the page colour and the touch glyph disappears, while the ring it replaces stays visible. Under @media (forced-colors: active), forcedColorAdjust: 'none' with a system colour such as CanvasText keeps it visible; the forced-colours state could join SpinnerTouch.a11y.chromium.spec.ts.
| /> | ||
| </svg> | ||
| </span> | ||
| <span aria-hidden="true" {...stylex.props(styles.glyph)}> |
There was a problem hiding this comment.
This glyph and its eight spokes are mounted on every Spinner, hidden under a fine pointer, so a Spinner renders 15 elements instead of 4. The component style authoring record's INV7 (minimal DOM) says states and variants use attributes, classes and styles on existing elements, never extra elements, so this needs either a DOM within INV7 or a recorded owner exception.
| usage: { | ||
| description: | ||
| 'An animated loading indicator for processes with unknown duration, such as data fetching or form submission. Supports visible labels, multiple sizes, and a dark background variant. For content with known dimensions, use Skeleton instead.', | ||
| 'An animated loading indicator for processes with unknown duration, such as data fetching or form submission. Supports visible labels, multiple sizes, and a dark background variant. On an iPhone or iPad it draws the iOS activity indicator (eight spokes with a fading trail) in place of the ring, at the same size and color. For content with known dimensions, use Skeleton instead.', |
There was a problem hiding this comment.
The glyph keys on (pointer: coarse) alone, so Android phones and other touch screens draw it too, and the default shade changes to the secondary text colour there. This line, :194, :224 and the --spinner-color default on :74 should say "under a coarse pointer" and mention the gray default; the Chinese --spinner-color entry needs the same update.
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [feat] Themes can retune touch motion roles. `defineTheme({motion})` takes `springs`, `durations` and `curves` keyed by motion role, the table path of a measured touch motion value (`springs.sheetPresent`, `navigation.transitionMs`), at the root or in an adaptation rule, and writes one `--motion-*` variable per role it sets. A role the theme leaves out keeps its measured default, an unknown role id is refused, and the duration scale (`fast`, `medium`, `ratio`) is optional when a theme sets only roles. No shipped component reads the roles yet. `astryx docs motion` lists every motion role with its CSS variable and default, and the theme authoring guide and template describe the new fields. |
There was a problem hiding this comment.
"No shipped component reads the roles yet" ships in the same release as Spinner reading progress.activityIndicator.cycleMs (.changeset/spinner-touch-activity-indicator.md); the CLI motion doc says the same at motion.doc.mjs:151. Name Spinner's cycle as the first role a shipped component reads.
| * and the velocity is 0.2 times the newest one plus 0.8 times the one | ||
| * before. Fitted to every value UIKit reported on six HID streams, exactly. | ||
| * It reacts a frame late to an acceleration, includes a hold before the | ||
| * first move, and never decays: a finger at rest sends no events, so a |
There was a problem hiding this comment.
The interaction modality record's INV10 (with AST-100 FR2), now current, asks for one release filter in which a contact held still past the stillness interval reads 0, and no alternative release filters. A filter that never decays, beside lsq2, iosFling and window, would let a sheet fling after a still finger; keep one filter with the stillness zero, or leave the tracker out with the stack.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the spec: the Spinner touch glyph (eight gray spokes under any coarse pointer, an 800 ms cycle that ignores reduced motion and keeps no shared phase, the progress.activityIndicator.cycleMs role and nine elements under touch) contradicts the current component:Spinner record (FR3, FR4, FR5, AR4) and needs an element-count exception under its minimal-DOM rules, and the description lists it as "Proposed; pending owner review". A maintainer needs to update component:Spinner for this intentional change, recording the touch presentation and that exception; any engineering owner, the author included, records the decision by commenting /approve-spec 742979eb5da69e385792625020ebdbab94614bd2 on this pull request. Whatever that decision is, these still need changes:
- Land or drop #8076 and #8177 and rebase, so this pull request carries only the touch glyph (32,285 authored lines today).
- Keep a Spinner visible without a stylesheet: under a coarse pointer the spokes draw nothing without CSS.
- Add real-device iPhone and Android recordings for the claims about iOS Safari and Android Chrome.
[Automated review]
| * (by default the theme's secondary text color, UIKit's gray; or a shade's) | ||
| * draws at UIKit's relative weights. | ||
| * | ||
| * Like UIKit's, the glyph keeps its pace under reduced motion, starts its own |
There was a problem hiding this comment.
This keeps the 0.8 s cycle under reduced motion and skips the phase pin, while current component:Spinner FR3 and AR4 slow the indicator to 3 s and FR4 keeps every mounted indicator in one phase. If the glyph keeps UIKit's behavior, that record needs the amendment an engineering owner approves.
| export function SpinnerTouchSpokes({size}: {size: SpinnerSize}) { | ||
| const glyph = GLYPH_STYLE[size]; | ||
| return SPOKES.map(spoke => ( | ||
| <span |
There was a problem hiding this comment.
Without a stylesheet these spans are empty, so a hydrated touch page whose CSS failed to load shows no loading indicator, where component:Spinner FR2 keeps the indicator drawing at its default geometry. An attribute-sized fallback, as the ring's svg has, or an FR2 exception in the record would cover it.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the spec: the Spinner touch glyph (eight gray spokes under any coarse pointer, an 800 ms cycle that ignores reduced motion and keeps no shared phase, the progress.activityIndicator.cycleMs role and nine elements under touch) contradicts the current component:Spinner record (FR3, FR4, FR5, AR4), needs an element-count exception under the minimal-DOM rules (AST-017 FR60), and is listed in the description as "Proposed; pending owner review". Approval needs a maintainer to provide the governing spec for this addition by amending component:Spinner to record the touch presentation and that exception; any engineering owner, the author included, records the decision by commenting /approve-spec 85698ae302e0f079cc5737b6dcfdfbf1a21cf3fc on this pull request. Whatever that decision is, these still need changes:
- Land or drop #8076 and #8177 and rebase, so this pull request carries only the touch glyph (30,560 authored lines today).
- Fix the Lint error at
packages/core/src/motion/useAnimator.test.tsxline 80, a component declared inside a test. - Keep a Spinner visible without a stylesheet: under a coarse pointer the spokes draw nothing without CSS.
- Add real-device iPhone and Android recordings for the claims about iOS Safari and Android Chrome.
[Automated review]
| vi.useFakeTimers(); | ||
| try { | ||
| const box: {current: Animator | null} = {current: null}; | ||
| function Probe() { |
There was a problem hiding this comment.
ESLint's @eslint-react/no-nested-component-definitions reports this Probe as an error, which fails the Lint workflow on this head. Moving it to module scope and passing it the box it fills keeps the test and clears the error.
| } | ||
|
|
||
| /** UIKit's activity indicator: eight spokes, the box's only children. */ | ||
| export function SpinnerTouchSpokes({size}: {size: SpinnerSize}) { |
There was a problem hiding this comment.
Under a coarse pointer these eight spans get their shape only from the stylesheet's masks, so with no CSS each Spinner measures 0 × 0 and draws nothing, where the ring keeps an attribute-sized svg (component:Spinner FR2: "With no stylesheet, the box and ring draw at the size's default geometry"). An attribute-sized fallback, or an FR2 exception recorded with the presentation decision, would close this.
|
Posted by Cindy's agent on her behalf. Cindy's decision: no new API for the iOS-style spinner. That means no new Spinner prop, no theme role, and no automatic glyph swap on touch screens; Spinner stays as it is today. If the spokes spinner is useful, add it as a story or example that composes it from existing APIs, so apps that want that look can copy it. Please drop the API from this PR. The example is optional. |
vjeux
changed the title
|
The API is out. f3b8dea returns Spinner, its tests and its docs to what is on its base (the ring on the compositor from #8177, below this branch) and removes the touch presentation, the |
There was a problem hiding this comment.
Changes are needed before this can merge. The example is the form an engineering owner asked for, but the branch still carries its whole stacked foundation: 30,879 authored lines across 170 files, including Spinner, theme and build changes. It conflicts with main in nine gesture and motion files, so no CI has run on it. Please rebase the story alone onto main, so the diff is just apps/storybook/stories/Spinner.stories.tsx.
- The stacked Spinner change (a rotating wrapper in place of the arc's moving dash) contradicts FR3 of the current Spinner record. It belongs with #8177 and an amendment to that record.
- The example sets
role="status"andaria-labelas it mounts, the pattern AST-086 FR2 and FR3 rule out. A copied indicator that appears after the page loads is usually not announced, so consider a non-live role such asrole="img"and a note that the app announces loading itself.
[Automated review]
| }) { | ||
| return ( | ||
| <span | ||
| role="status" |
There was a problem hiding this comment.
Apps are told to copy this component, and a live region mounted already holding its name usually isn't announced when it appears after the page loads (AST-086 FR2, FR3). A non-live role such as role="img" with this aria-label, plus a note that the app announces loading through its own persistent live region, would keep the copy accessible.
| /> | ||
| </svg> | ||
| <span | ||
| ref={syncRotationPhase} |
There was a problem hiding this comment.
There was a problem hiding this comment.
Changes requested: the story itself is close, but this branch also carries #8177’s Spinner rotor and a restack of #8076’s touch foundation, which make up 22,957 of its 23,088 changed lines. Please rebase so the diff is the story alone, onto main or onto #8177 once it lands. Before the rotor lands anywhere, an engineering owner needs to amend component:Spinner FR3, which describes one SVG with dash motion. In the story, render the indicator as a named non-live element (for example role="img" with the label) instead of a role="status" element mounted with its name, per spec:AST-086 FR2 and FR3.
[Automated review]
| }) { | ||
| return ( | ||
| <span | ||
| role="status" |
There was a problem hiding this comment.
This root mounts as role="status" already named “Loading”, the shape spec:AST-086 FR3 rules out (“It MUST NOT mount that element already holding the text to announce”) and that component:Spinner records as its own AR1 gap. Since apps copy this, consider a named non-live element (for example role="img" with aria-label) and a note that the app announces loading through its own channel, such as useAnnounce.
There was a problem hiding this comment.
Fixed in 2883466: ActivityIndicator is now a named image (role="img" with aria-label), not a live region, and its comment says the app announces loading once through the shared channel (useAnnounce from @astryxdesign/core/hooks).
| const dashMotion = stylex.keyframes({ | ||
| '0%': {strokeDashoffset: '0px'}, | ||
| '100%': {strokeDashoffset: `calc(var(${RESOLVED_DIAMETER}) * -${PI})`}, | ||
| const rotation = stylex.keyframes({ |
There was a problem hiding this comment.
This rotation comes from #8177 and replaces the arc’s dash motion with a rotating span holding a second SVG. component:Spinner FR3 (current) describes “an aria-hidden SVG of two concentric circles” whose “arc’s dash moves around the stationary circle”, so it needs an engineering owner’s amendment before it lands, and it should not ride in this docs-only branch.
There was a problem hiding this comment.
The rotor no longer rides in this branch, which is now this story alone on main. The FR3 amendment it needs is asked on #8177: #8177 (comment)
…g APIs A story for an app that wants iOS's look: `ActivityIndicator`, eight capsule spokes drawn with StyleX and plain elements, to copy. As UIActivityIndicatorView draws it on iOS 26: a bright head stepping clockwise one spoke every 100 ms (a turn every 0.8 s) and the trail dimming behind it, medium 20 pt and large 37 pt, in UIKit's secondary label gray at its images' alphas, light and dark. Each spoke runs one opacity animation whose holds are joined by short linear ramps rather than steps(), so WebKit runs it on the compositor. It is a named image, not a live region (`spec:AST-086` FR2, FR3): the app announces loading through the shared channel. Spinner itself is unchanged.
1e0cc60 to
2883466
Compare
github-actions
Bot
removed
the
needs:design-review
There was a problem hiding this comment.
Recommended for approval: the story is the composed example an engineering owner asked for, it adds no Spinner API, and ActivityIndicator is a named image with no live role, as the status-announcement spec expects. Two optional touches: Storybook's code panel for this story shows only its render, so naming apps/storybook/stories/Spinner.stories.tsx in the story description would help readers find the component to copy, and in light mode the brightest spoke measures 2.75:1 on white, so apps that need 3:1 may want the darker theme color the comment mentions.
[Automated review]
|
|
||
| /** | ||
| * The iOS look, composed from existing APIs: copy `ActivityIndicator` from | ||
| * this story's source into an app that wants it. |
There was a problem hiding this comment.
Optional: in the built Storybook, this story's code panel shows only the story object (a render of two <ActivityIndicator /> elements), not the component, keyframes and styles above, so "this story's source" is not something a Storybook reader can copy from. Naming the file here (apps/storybook/stories/Spinner.stories.tsx) would point them to it.
…contrast Storybook's code panel shows only a story's render, so the story's description names the file that holds `ActivityIndicator`. The color comment no longer says any theme color works: UIKit's gray reaches 2.74:1 at the brightest spoke on a white page, and the theme's secondary text color is lighter still (2.25:1); the theme's primary text color reaches 3.64:1 at the same alphas, for an app that needs WCAG's 3:1 for non-text graphics. No pixel changes.
|
Both optional touches are in d3c0757. The story's description now names |
There was a problem hiding this comment.
Recommended for approval: the story is the composed example an engineering owner asked for, it adds no Spinner API, and ActivityIndicator is a named image with no live role, as the status-announcement spec expects. The story's description now points readers to the file to copy, and the color comment states the light-mode contrast with a 3:1 alternative, so nothing more is needed here.
[Automated review]
…er an unhandled rejection (#8478) ## User impact A bottom-sheet DropdownMenu preloads its action list at rest. When that load fails (a dropped or flaky connection), the page gets an unhandled promise rejection in the console. The rejected load also stays cached, so every later open of the menu fails to show its rows until a reload. In this repository's CI, the same unhandled rejection fails the UI test shard that runs `DropdownMenu.ssr.test.tsx` while every test passes, as on #8353, #8229 and #8211. ## Problem and solution fit - Why does it occur? `preloadMenuBottomSheetActionList` starts the load with `void loadActionList()`: nothing handles a rejection, and `actionListLoad ??=` keeps the rejected promise. - Why does it harm the task? An unhandled rejection is reported as an error, and a cached rejection turns one network blip into a menu whose rows never appear. - Why does that matter? The preload only exists to make the first open instant; it must never break the open. In tests, the SSR file can finish while its preload is still importing; the environment tears down mid-import and the rejection fails the run. The fix handles the preload's failure and drops the failed load, so the next preload or open asks the loader again. The SSR test waits for the sheet modules before it ends. ## Expected behavior and authority The sheet loads on demand (#7709) and preloads at rest so the first open shows its actions at once (#8392). A preload is an optimization: its failure surfaces no error and blocks no retry. ## Smallest restoration - `loadActionList` clears its cached load when the import rejects. - `preloadMenuBottomSheetActionList` handles the rejection. - `DropdownMenu.ssr.test.tsx` awaits the two sheet modules after unmounting. No API, rendering or timing change. ## Evidence - Reproduction before the change: on main e9b3f3e, `DropdownMenu.ssr.test.tsx` run alone fails 5 of 6 runs with three `EnvironmentTeardownError`s each, and the new unit test records the unhandled rejection. - Result after the change: the SSR file passes 8 of 8 runs alone, and the unit test passes. - Representative unchanged path: all 10 DropdownMenu test files (312 tests) pass. - Regression test or other mutation-sensitive proof: `lazyMenuBottomSheet.test.ts` fails the list's import once, then checks there is no unhandled rejection and that the next call imports it again. Keeping the rejected load cached fails it ("expected 1 to be 2"). ## Scope - [x] One defect is restored. - [x] Separable contradictory or unsettled tagalongs were removed or split. - [x] No new public API, visual representation, default, or interaction model is hidden under the bug-fix label. - [x] Public text and artifacts contain no internal Meta context. ## Testing - At 3962530: the changeset and comments promise only what holds everywhere (no unhandled rejection, no kept failure); the unit and SSR tests: 2 passed; changeset coverage: passes. - At 8877d94 (adds the Changeset): `node scripts/release/changeset-coverage.mjs pr --base e9b3f3e` fails at 4d8e178 and passes here (`[fix]`, `@astryxdesign/core`); `pnpm check:changesets`: valid. - `pnpm exec vitest run --project ui packages/core/src/DropdownMenu/lazyMenuBottomSheet.test.ts`: fails on main, passes here. - `pnpm exec vitest run --project ui packages/core/src/DropdownMenu/DropdownMenu.ssr.test.tsx`, alone, 8 times: 8 of 8 (main: 1 of 6). - `pnpm exec vitest run packages/core/src/DropdownMenu`: 10 files, 312 tests. - `tsc -p packages/core/tsconfig.json --noEmit`, eslint and prettier on the changed files: clean.
…oined (#9070) ## Exact decision needed Whether `spec:AST-086` FR9's shared channel joins a component's superseded message about its current state with its next one, and the optional public `useAnnounce` option that asks for replacement. Today every message written to a region before the previous one is exposed is joined, so a component that states its current state on every input, such as a search's result count, has all its stale states spoken together when input outpaces a frame (a barcode scanner, dictation, automation): `1 result Create "Urg" Create "Urge" Create "Urgen" Create "Urgent"`. ## Existing authority searched - `spec:AST-086` FR9 (the join), FR6 (an identical message repeated in a frame is one message), FR8 (a busy start its outcome overtakes is dropped), FR13 and FR14 (own-content messages and modal containers). - `architecture:interaction-modality` INV8: superseded work must not change the current status or announcement, with supersession left to component contracts. - `spec:AST-075` DEC-2 and FR7: Astryx's comboboxes announce their result count with `useAnnounce`, the same hook products use. Tokenizer's record: its sheet's count and empty-state text go through `useAnnounce`. MultiSelector's record (draft): its result count and create row label go through the polite channel. - `spec:AST-002` FR1, FR2, FR5, FR7, FR16 and FR20 for the new public option; `has*` booleans such as `hasClear` and `hasPaging` for its name. - Open pull requests that mention AST-086 (#7871, #7902, #7910, #8175, #8211, #8583, #8001): none edits FR9. #8640 implements the replacement for MultiSelector and waits for this decision. ## Decision A `useAnnounce` instance created with `hasReplace` states one current state of its owner, such as a search's result count: each message it writes withdraws that instance's previous message while it is not yet exposed, whatever their urgencies, and is spoken in the region of its own urgency; a message already exposed has been heard and stays. Messages from other instances, and from instances without `hasReplace`, still join. `useAnnounce` takes an optional options object with one field, `hasReplace` (default `false`), and the `announce(message, politeness)` it returns is unchanged. FR14's modal containers withdraw the same way, and a picker's own empty-panel message comes from its search's `hasReplace` instance (FR13). Astryx's search counts and MultiSelector's create row use a separate `useAnnounce({hasReplace: true})` instance (`spec:AST-075` FR7, Tokenizer's and MultiSelector's records); selection messages keep their own plain instance. DEC-4 records it; its decider is the approving engineering owner, with INV8 and FR8 as its reasons. Public option (`spec:AST-002` FR5, FR20): two otherwise identical messages, `"7 results"` written twice in one frame, need different outcomes. As a search's count it replaces the earlier one; as two distinct notices both are spoken. Only the caller knows which it sends, and the channel cannot derive it. Before: `const announce = useAnnounce(); announce(t('resultCount', {count}));`. After: `const announceCount = useAnnounce({hasReplace: true}); announceCount(t('resultCount', {count}));`. Existing calls are unchanged. ## Rejected alternatives - Each owner debouncing its own messages: every announcement waits, and every owner repeats the rule. - An exception for every `useAnnounce` message: Astryx's comboboxes and Tokenizer announce through `useAnnounce`, so their counts would keep joining. - A string key on each message (`announce(message, politeness, {currentState})`): a third argument whose strings every call must keep consistent, where one instance already states one state. ## Claim scope and non-goals FR9's replacement within one `hasReplace` instance, the option that asks for it, and the AST-075, Tokenizer and MultiSelector sentences that describe their counts' announcement. Which messages a component sends through a `hasReplace` instance stays with its own record. ## Evidence and implementation relationship - On main, the MultiSelector CreateRow accessibility spec, which types into the create row at about four keystrokes per frame, fails 9 of 10 runs alone and 26 of 48 under load, its polite region reading the joined statuses. - #8640 implements the replacement for MultiSelector. With it, the spec passes 10 of 10 alone and 48 of 48 under load. It lands after this record changes, adopting `useAnnounce({hasReplace: true})`. ## Writing rubric results - R1 narration: none, in AST-086, AST-075, Tokenizer's or MultiSelector's record. - R2 research in the record: none. - R3 evidence naming a change: none. - R4 rejected alternatives over one line or outside a decision: one, inside DEC-4, one line; nothing added to the other records. - R5 pull request or issue references remaining: none. - R6 ruled open questions still listing options: none. ## Scope - [x] This PR contains specification records only. - [x] It records observable behavior and preserves equivalent internal implementations. - [x] Any named internal mechanism is intentionally public, names its dependent caller/system, and explains why an equivalent implementation would not satisfy the contract. - [x] Verification states the evidence layer and failure signal without prescribing CI job/workflow topology. - [x] It records one intentional durable decision, not a rescue for a separable tagalong. - [x] The canonical knowledge owner, affected current claims, and overlapping open work are named; any ownership collision is resolved or called out for human decision. - [x] Adjacent API, visual, accessibility, composition, and implementation facts remain outside scope unless this exact decision depends on them. - [x] Public text and artifacts contain no internal Meta context. ## Validation Local gates at 2b84901: `pnpm check:knowledge` (records and templates aligned), Prettier, `git diff --check`, `pnpm check:repo`.
Reader impact
An app that wants iOS's activity indicator finds no Spinner option: Spinner draws a ring everywhere. A new story shows how to build that look from StyleX and plain elements, as a component to copy.
Source of truth
What led to it: making Astryx match iOS under a finger (
spec:AST-094), this PR first added a Spinner option swapping in the iOS look on touch screens; cixzhang decided against any new API and asked for a composed story instead.Options:
The drawing follows UIActivityIndicatorView on iOS 26.2, measured on the simulator: eight capsule spokes 45° apart, the head stepping clockwise every 100 ms, alphas from 0.51 to 0.165 in the secondary label gray, medium 20 pt and large 37 pt, unchanged under Reduce Motion and right to left.
Correction
ActivityIndicatorinSpinner.stories.tsx, eight spans with one opacity animation each, joined by short linear ramps rather thansteps()so WebKit keeps them on the compositor; CanvasText under forced colors.role="img"with a label), not a live region (spec:AST-086FR2, FR3): the app announces loading once, throughuseAnnounce.Evidence
UIKit (iOS 26.2), and the story's first image in Chromium, medium and large:
The story running in Mobile Safari on the iOS 26.2 simulator:
Chromium, each 100 ms of a turn:
Scope
Testing
What I ran and what it showed: