Repository navigation
feat(ProgressBar): under reduced motion the indeterminate bar pulses in place - #8239
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🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| ProgressBar · Storybook | 10.7 kB (+325 B) · 7.3 kB CSS (+114 B) | 568 (+44) | — · — · — |
Package size: @astryxdesign/core 5.92 MB packed (+1.8 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: core 1 row; run pnpm api:report.
Wide change (shared styles: packages/core/src/theme/derivedVarRegistry.test.ts): add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
Generated by PR Enrichment workflow | 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 ProgressBar touch change needs an engineering-owner decision before it can be approved: it rests only on the draft spec:AST-094, so an engineering owner, the author included, can record approval of that direction (the touch indeterminate sweep, its Reduce Motion pulse and its timing outside the theme's motion tokens) by commenting /approve-spec <full head SHA>, or point to another governing spec. Whatever that decision, two changes are needed:
- Land the stacked predecessors first (or retarget) so this pull request carries only its own 461-line commit instead of 16,939 authored lines.
- Under a coarse pointer, a value that reverses to the one it just left animates in half the stated time (400 ms instead of 800 ms), because CSS shortens reversed transitions.
[Automated review]
6efa969 to
33a8e0f
Compare
There was a problem hiding this comment.
This ProgressBar touch change needs an engineering-owner decision before it can be approved: it rests only on the draft spec:AST-094, so an engineering owner (the author included, if they are one) can record approval of that direction (the touch sweep, its trackless Reduce Motion pulse and its timing outside the theme's motion tokens) by commenting /approve-spec <full head SHA>, or point to another governing spec. Whatever that decision, these changes are needed:
trackIndeterminatesetsdefault: null, which removes the grey track from every indeterminate ProgressBar, with a mouse too; keep the default colour in the override and test the mouse path.- Land the stacked predecessors first (or retarget) so this pull request carries only its own 500-line commit instead of 16,976 authored lines.
- Under a coarse pointer, a value that reverses to the one it just left animates in half the stated time (400 ms instead of 800 ms).
- AST-094 FR7 amends
spec:AST-090, which is not in the repository.
[Automated review]
github-actions
Bot
removed
the
needs:design-review
There was a problem hiding this comment.
This ProgressBar change needs an engineering-owner decision before it can be approved: it rests only on the draft spec:AST-094, so an engineering owner (the author included, if they are one) can record approval of that direction for the iOS bar (the quarter-track sweep, the trackless Reduce Motion pulse and its theme-retunable timing) by commenting /approve-spec <full head SHA>, or point to another governing spec. Whatever that decision, these changes are needed:
trackIndeterminatekeepsdefault: null, which removes the grey track from every indeterminate ProgressBar, with a mouse and on Android too; keep the track colour as the default and test those paths.- Land the stacked predecessors first (or retarget) so this pull request carries only its own two commits instead of 19,658 authored lines.
- The iOS gate infers the engine from
-webkit-touch-callout, whichspec:AST-013FR6 does not allow, and the themeplatformcondition is outsidespec:AST-012FR2's closed vocabulary, so each needs its record amended first or has to go. - AST-094 FR4 and DEC-1 say touch motion is not themable while the new theme motion roles retune it, the motion docs advertise roles no component reads, and FR7 amends
spec:AST-090, which is not in the repository. - On iOS a fill that reverses to its previous value runs at half its written duration,
lintandtest-node (1/4)fail on a missing@astryxdesign/clichangeset and a stale docs golden, and the description needs AST-094's writing-rubric results.
[Automated review]
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on draft spec:AST-094 for the iOS indeterminate bar, including its iOS engine gate, which spec:AST-013 FR6 does not allow today, and its theme-retunable timing, which the draft's own FR4 rules out. An engineering owner records that decision by commenting /approve-spec b7432c040517a75d25a5827fb880aecf97c2425b on this pull request, and the author may do so as an engineering owner.
- Restore the indeterminate track, which the
nulldefault atProgressBar.tsx:281-290removes with a mouse and on Android too. - Rebase onto the stacked predecessors so this pull request carries only its own commits, drop
when.platformor amendspec:AST-012FR2, fix the halved iOS reversal, and keep the motion docs to roles that have a reader.
[Automated review]
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about draft AST-094. That decision covers the iOS-only engine gate (current AST-013 FR6 does not allow it), the trackless Reduce Motion pulse, and theme-retunable touch roles, which AST-094 FR4 and DEC-1 forbid as written. An engineering owner records it by commenting /approve-spec <full head SHA> on this pull request, and you may do so yourself if you are an engineering owner. Whatever the decision, these still need changes:
- With Reduce Motion on iOS the pulsing bar reaches only 1.52:1 to 2.42:1 against the page, below the 3:1 non-text contrast AST-020 requires.
- Rebase onto the stacked predecessors so the pull request carries only its own four commits, about 640 of its 19,658 authored lines.
- Drop
when.platformor amend AST-012 FR2 first, derive the Spinner spoke delays from the cycle role, keep a reversed iOS move at its documented duration, document only motion roles something reads, and fix the citation of the missing AST-090.
[Automated review]
…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 needs an engineering owner to approve or identify the governing spec for the iOS Safari look of ProgressBar's indeterminate bar, which today rests only on draft spec:AST-094: a 25% linear sweep, a Reduce Motion pulse with no track, whether it applies on iOS only or on every touch screen, and whether themes may retune its timing. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision, please rebase onto the landed stack so this diff is the ProgressBar change alone, and fix these:
- Use the shared compact definition from
spec:AST-012FR9 instead of 744 px, which opens adaptive menus as popovers on 744–767 px touch screens. - Key the touch styles on the shared touch resolver instead of
-webkit-touch-callout, asspec:AST-013FR6 and the owner comment ask. - Drop
when.platform, the theme-settable reduced-motion fade andreducedMotion: 'keep', whichspec:AST-012FR2 and the reduced-motion design record rule out. - Make the motion docs describe only roles a component reads, reject unknown role ids, and fix AST-094's citation of the missing AST-090.
[Automated review]
9f35484 to
c234eaf
Compare
vjeux
changed the title
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested: the touch timing and the reduced-motion pulse match the ProgressBar record and their tests pass, but three small fixes are needed first.
- With forced colors under a coarse pointer, the new timing drives the fill's
widthtransition (800 ms, linear), which AST-094 FR5 rules out; land the sized fill at once there and add a forced-colors test. - Remove the dependency-free
useEffectthat callsregisterProgressBarTable()inProgressBarTouch.ts, since nothing reads the registry at runtime. - Scope the reduced-motion changeset's 3:1 promise to the built-in tokens: in the Neutral, Butter and Matcha themes, some tones fall below 3:1 at the 0.84 floor.
[Automated review]
c234eaf to
396791a
Compare
There was a problem hiding this comment.
Ready to approve: the touch timing and the reduced-motion pulse match the ProgressBar record, and the forced-colors fill, the extra Effect and the changeset wording from the last review are all fixed. The new Chromium specs and the unit tests pass. The inline notes are optional comment cleanups (a FillStyle comment that drifted onto PulseStyle, one long comment and one long file header).
[Automated review]
396791a to
e1e83a3
Compare
There was a problem hiding this comment.
Ready to approve: the touch fill timing and the reduced-motion pulse match the ProgressBar record, and with forced colors under a finger the sized fill still lands at once now that its rule sits under the touch key. The new Chromium specs and unit tests pass on this head. The inline notes are optional: a stale "two roles" count in the motion README and three comment trims. The repository's spec gate still needs an engineering owner (the author included, if they are one) to comment /approve-spec <full head SHA>, because the pull request touches two spec files.
[Automated review]
e1e83a3 to
753c92c
Compare
|
Open review threads at 753c92c (this pull request's own change is its last commit), by disposition:
I resolve the fixed and moot threads. |
|
/approve-spec 753c92c |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested: the reduced-motion pulse and the touch fill timing match the current ProgressBar record and their tests pass, but one doc comment still describes the old behaviour.
- The
isIndeterminateJSDoc inProgressBar.tsx(line 124) still says the bar "Respectsprefers-reduced-motionby slowing the animation"; under reduced motion it now pulses its opacity in place instead of sliding, so update that line to matchProgressBar.doc.mjs.
The branch also needs a rebase onto main: packages/core/src/motion/MEASUREMENTS.md conflicts there, so keep main's paging feed for the M-PG1 row. The inline notes are optional comment trims. An engineering owner approved the spec change on this head; after the fix, an engineering owner (the author included, if they are one) comments /approve-spec <full head SHA> again on the new head.
[Automated review]
…in place Under reduced motion, on every pointer, the indeterminate fill spans the whole track as one solid bar and only its opacity pulses, from 1 down to 0.84 and back, each way taking the progress.reducedMotionPulseMs role (2000 ms) on the timing.curves.move curve; tempo does not scale it. The 0.84 floor keeps every tone that reaches 3:1 against the track at full opacity at 3:1 in the light and dark default themes (component:ProgressBar FR6, AR4).
cixzhang
added
the
ci:run
753c92c to
d6d738c
Compare
|
/approve-spec d6d738c |
There was a problem hiding this comment.
Approve: under reduced motion the indeterminate bar now pulses in place as the ProgressBar contract describes (full width, opacity 1 to 0.84 and back, 2 s each way, unaffected by tempo), and at the 0.84 floor every tone that reaches 3:1 against the track keeps it in the default light and dark themes.
Optional comment tidy-ups (non-blocking), following the source-comment guideline:
ProgressBar.tsx:513and the JSDoc at:535-538restate the code next to them; consider deleting both.ProgressBar.tsx:372-376and:177-180can keep only the reasons: the values are set inline from the progress table, tempo does not scale this loop, and the keyframes run one way whilealternateplays them back.
[Automated review]
| // Under reduced motion the indeterminate fill stops travelling and pulses in | ||
| // place. It goes from full opacity down to the pulse's floor | ||
| // (`--_progress-bar-pulse-floor`, the progress table's) and back. The | ||
| // animation alternates direction each pass. |
There was a problem hiding this comment.
Optional (comment style): the middle sentence narrates the keyframes. A shorter form that keeps the reason:
// Under reduced motion the fill pulses in place instead of travelling.
// The keyframes run one way, and `alternate` plays them back.| // Under reduced motion, on every pointer, the fill spans the whole track | ||
| // as one solid bar, and only its opacity pulses. Each way takes the | ||
| // `progress.reducedMotionPulseMs` role on the `timing.curves.move` curve | ||
| // (`--_progress-bar-pulse`, `--_progress-bar-pulse-curve`, set inline from | ||
| // the progress table). A busy loop: tempo does not scale it. |
There was a problem hiding this comment.
Optional (comment style): the first sentence restates the width and animationName keys below. Keep only the reasons:
// The pulse's duration, curve and floor are set inline from the progress
// table. Tempo does not scale this busy loop.| }, | ||
| }); | ||
|
|
||
| // The reduced-motion pulse's variables, from the progress table. |
There was a problem hiding this comment.
Optional (comment style): this restates PulseStyle's name and members; delete it.
| /** | ||
| * The reduced-motion pulse plays the `progress.reducedMotionPulseMs` role on | ||
| * the `timing.curves.move` curve, down to the progress table's floor. | ||
| */ |
There was a problem hiding this comment.
Optional (comment style): this JSDoc on an unexported helper restates its three lines; delete it.
User impact
With reduced motion on, an indeterminate ProgressBar still slides a 40% segment, 3 s a pass.
design:reduced-motionDR1 says nothing should travel there, so the bar now stays put: on every pointer its fill spans the track and only its opacity pulses, from full to 0.84 and back, 2 s each way.Options and tradeoffs:
Visual authority
DDrow reused or amended.Design decisionstable by a team design owner.docs/design/.Settled by
component:ProgressBarFR6, AR4 and DEC-2: the pulse plays theprogress.reducedMotionPulseMsrole ontiming.curves.move, which tempo does not scale.Before and after
Reduced motion, Chromium, one bar seeked through a cycle; before (main) then after:
State matrix
Writing rubric results
component:ProgressBar(FR6's gap lines, the FR6 and AR4 verification rows) and the generated role table: R1–R6 none; the rows name the new tests.Scope
Testing
Rebased onto main after #8225 merged; comments carry no record ids. One summary comment maps every open bot thread; the fixed and moot ones are resolved.
Local gates at d6d738c: the core unit suite (18,382); ProgressBar browser specs 43/43 and ×3 on two pinned cores 129/129 (Chromium-only); typecheck, lint, every check and the a11y contract (1,008) pass. Global baselines, run by hand: 177 claims matched.