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
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request is waiting on a spec decision. Its branch adds the draft spec:AST-094 (FR1–FR23, DEC-1, DEC-2), which also proposes amending current architecture:theme-tokens, design:user-states DR1 and DR3, family:buttons and spec:AST-025 FR14. An engineering owner needs to approve or reject it, and update or decline those records, by commenting /approve-spec <full head SHA> here; you can record that yourself if you are an engineering owner. Whatever that decision, these points need changes:
- Restack once the predecessors land, so this pull request carries only the reduced-motion commit instead of 41 commits and about 29,000 authored lines.
- Button and the three cards now import
../motion/reducedMotion.stylex, which core's exports map does not list, soastryx swizzleproduces an unresolvable import and CI'stest-node (4/4)fails (swizzle.test.mjs:153). - On a coarse pointer,
design:reduced-motionDR2 asks for an opacity-only fade, but Button and the cards still transition colours, borders and shadow (Button.tsx:113–114,SelectableCard.tsx:80), and the DR2 test checks only durations. - AST-094 FR8 (a 0.5 s navigation fade, sheets and menus on springs, a throw that keeps moving) contradicts
design:reduced-motionDR1 and DR2 and should be conformed to them.
[Automated review]
| import type {BaseProps} from '../BaseProps'; | ||
| import type {Elevation, SizeValue} from '../utils/types'; | ||
| import * as stylex from '@stylexjs/stylex'; | ||
| import {reducedMotionFadeConsts} from '../motion/reducedMotion.stylex'; |
There was a problem hiding this comment.
astryx swizzle rewrites this import to @astryxdesign/core/motion/reducedMotion.stylex, but core's exports map does not list that subpath, so a swizzled Button (and ClickableCard, SelectableCard, Thumbnail) gets an import that does not resolve. CI's test-node (4/4) fails on it at packages/cli/api/swizzle/swizzle.test.mjs:153. Moving the constants into an exported .stylex module (for example utils/interactionOverlay.stylex.ts, which already holds pressConsts) would fix it.
| default: 'background-image, background-color, color, opacity, transform', | ||
| // Under reduced motion nothing scales, so nothing transitions transform. | ||
| '@media (prefers-reduced-motion: reduce) and (pointer: coarse)': | ||
| 'background-image, background-color, color, opacity', |
There was a problem hiding this comment.
Under (prefers-reduced-motion: reduce) and (pointer: coarse) this keeps background-image, background-color and color transitioning for 200 ms. design:reduced-motion DR2 requires these transitions to "change opacity only", and its verification row fails "a property other than opacity". The policy's own example in motion/reducedMotion.ts (lines 24–28) uses 'opacity' alone for this key.
| @@ -77,8 +78,22 @@ const styles = stylex.create({ | |||
| ':is(:disabled,[aria-disabled="true"])': 'default', | |||
| }, | |||
| transitionProperty: 'box-shadow, border-color', | |||
There was a problem hiding this comment.
With the new coarse-pointer duration at lines 89–90, box-shadow and border-color now animate for 200 ms under reduced motion, but DR2 allows only opacity. Carrying the change on opacity (for example, fading the existing ::after by opacity) would conform.
| on: a transition that travels becomes a cross-fade carrying the same change | ||
| of state. An entrance lands in place and fades in, an exit fades out in | ||
| place, and any other move fades through. A navigation push or pop | ||
| cross-fades over 0.5 s on the ease-in-out curve; a sheet, a full-screen |
There was a problem hiding this comment.
FR8 asks for a 0.5 s navigation cross-fade, sheets that fade on their presentation spring, menus on a Reduce Motion spring, and a throw that keeps its motion. Touch presentations key on a coarse pointer (FR13), where the current design:reduced-motion record requires an opacity-only change of 200 ms or less, including for a transition the release of a touch drag starts (DR2), and forbids springs (DR1). Please conform FR8 to DR1 and DR2.
| try { | ||
| const {element} = await transitionsOf(button.page, 0); | ||
| expect(element).toBeGreaterThan(0); | ||
| expect(element).toBeLessThanOrEqual(200); |
There was a problem hiding this comment.
This proves the length of the DR2 fade but not that it is opacity-only. Also reading transitionProperty on the element and on its ::after, and expecting only opacity, would catch the colour transitions at Button.tsx:113–114.
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [feat] Themes can retune touch motion. `defineTheme({motion})` takes `springs`, `durations` and `curves` keyed by motion role, the table path of a value touch presentations play (`springs.sheetPresent`, `timing.reducedMotionCrossFadeMs`), at the root or in an adaptation rule. A role the theme leaves out keeps its measured default, and the duration scale (`fast`, `medium`, `ratio`) is optional when a theme sets only roles. Adaptation rules gain a `platform` condition, `ios` or `other`. `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.
timing.reducedMotionCrossFadeMs is excluded from the roles (motion/uikit/timing.ts:158), and .changeset/reduced-motion-record.md says it is no longer a theme role. Naming it here tells theme authors they can lengthen the Reduce Motion fade, which design:reduced-motion DR4 forbids.
| @@ -247,9 +255,15 @@ export interface DefineThemeInput { | |||
| * // Snappy: { fast: 100, medium: 250, ratio: 0.75 } | |||
| * // Default: { fast: 175, medium: 410, slow: 975, ratio: 0.75 } | |||
| * // Cinematic: { fast: 200, medium: 500, slow: 1200, ratio: 0.7 } | |||
| * | |||
| * // A softer sheet and a slower reduced-motion fade: | |||
There was a problem hiding this comment.
The example sets navigation.transitionMs, but its caption promises "a slower reduced-motion fade", which DR4 rules out and the code no longer allows.
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
65b726d to
ffc29bb
Compare
ffc29bb to
474f6e3
Compare
474f6e3 to
f639ec3
Compare
There was a problem hiding this comment.
This pull request is waiting on a spec decision: its branch adds the draft spec:AST-094 (FR1–FR23, DEC-1, DEC-2), which proposes amendments to current architecture:theme-tokens, architecture:interaction-modality, design:user-states DR1 and DR3, family:buttons and spec:AST-025 FR14, and a new stable @astryxdesign/core/motion/reducedMotion.stylex export that its own code marks as pending owner review. An engineering owner needs to approve or reject them by commenting /approve-spec <full head SHA> on this pull request; you can record that yourself if you are an engineering owner. Whatever that decision, these points need changes:
- Restack once the predecessors land, so this pull request carries only the reduced-motion commit instead of 69 commits and about 30,600 authored lines.
- On a coarse pointer,
design:reduced-motionDR2 asks for an opacity-only fade, but Button and the cards still transition colours, borders and shadow (Button.tsx:113–114,SelectableCard.tsx:80), and the DR2 test checks only durations. - AST-094 FR8 (a 0.5 s navigation fade, sheets and menus on springs, a throw that keeps moving) contradicts DR1 and DR2, as do the
springs.menuReducedMotiontheme role and the animator changeset's spring fade. - The theme template documents a
platformadaptation axis thatdefineThemenow rejects (packages/cli/assets/theme.template.ts:361–362).
[Automated review]
| * `from` includes its point; `below` excludes it. The other supported axes | ||
| * are primary-pointer precision, contrast preference, and motion preference. | ||
| * are primary-pointer precision, contrast preference, motion preference, and | ||
| * platform (`ios` for the browsers of iPhone and iPad, `other` for the rest). |
There was a problem hiding this comment.
This sentence documents a platform adaptation axis, but the stack removed it: CONDITION_AXES in packages/core/src/theme/themeAdaptations.ts ends at motion, and assertAllowedKeys throws ….when.platform is not supported. A theme author who follows the template gets an error from defineTheme. Restoring the base wording ("primary-pointer precision, contrast preference, and motion preference") would fix it.
| * iPhone 17 Pro, `_animateUsingSpringWithDampingRatio:response:tracking:` | ||
| * hooked with Reduce Motion on (1.0, 0.4), Oct 23 2026 | ||
| */ | ||
| menuReducedMotion: {response: 0.4, dampingRatio: 1}, |
There was a problem hiding this comment.
springs.menuReducedMotion is registered as a theme motion role (it appears in astryx docs motion as --motion-springs-menu-reduced-motion). design:reduced-motion DR4 says a theme value offers no way to keep springs under reduced motion, and DR1 forbids springing; excluding it from the roles, as timing.ts does for reducedMotionCrossFadeMs, would keep the theme surface within DR4.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] The touch presentations' animator plays any closed-form curve (such as a spring chasing a moving target), drives a pseudo-element such as a dialog's ::backdrop, fades on a presentation's own spring under Reduce Motion (with an entrance or exit role for a slide that changes no opacity), and starts a call from rest when the browser commits it, as Core Animation does. Not exported from the package root; touch presentations reach it through the experimental subpaths (see their entry). |
There was a problem hiding this comment.
This release note says the animator "fades on a presentation's own spring under Reduce Motion", while .changeset/reduced-motion-record.md says it cross-fades in 200 ms and removes call options that no release ever shipped. Both land in the same release; describing only the final behaviour would keep the notes consistent with design:reduced-motion DR1 and DR2.
| @@ -177,6 +190,13 @@ const STATIC_EXPORTS = { | |||
| types: './dist/utils/interactionOverlay.stylex.d.ts', | |||
| default: './dist/utils/interactionOverlay.stylex.js', | |||
| }, | |||
| // Proposed; pending owner review (spec:AST-094 OQ4): the Reduce Motion | |||
There was a problem hiding this comment.
This new stable export is justified by "spec:AST-094 OQ4", but the record has only OQ1 and OQ2. The subpath sits outside any experimental boundary, so once released it falls under the stable compatibility promise; it needs an owner decision, and the citation should point at what actually covers 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.
spec:AST-090 does not exist on main or on this branch, so FR7's "departs from the draft spec:AST-090 FR1 … and FR10" (and the same citation under Current-state impact) points at nothing an owner can read.
| const read = new Set( | ||
| [ | ||
| ...css.matchAll( | ||
| new RegExp(`var\\(\\s*(--(?:${names})[a-z0-9]+)\\s*[,)]`, 'g'), |
There was a problem hiding this comment.
Matching --(?:astryx|x)[a-z0-9]+ also catches a product's own variables such as --xl or --xsmall read from a StyleX style and declared in global CSS; the build then warns that the declaration "would be dropped in every browser", or fails with undefinedVariables: 'error'. Deriving the generated names from the StyleX rules the build collected would avoid the false report.
f639ec3 to
f876650
Compare
There was a problem hiding this comment.
This pull request is waiting on an engineering-owner decision: its stacked commits add public theme fields (motion.springs, motion.durations, motion.curves), an undefinedVariables option for astryxStylex, and an internal touch animator, gesture system and interaction harness, none of which a current spec covers. Approval needs an engineering owner to identify or provide the governing spec for these additions, or to approve them as proposed, by commenting /approve-spec <full head SHA> here; you can record that yourself as an engineering owner. Whatever that decision, these points need changes:
- Restack once the predecessors land, so this pull request carries only the reduced-motion commit instead of about 29,700 authored lines.
- On a coarse pointer,
design:reduced-motionDR2 asks for an opacity-only fade, but Button and the cards still animate colour, border and shadow (Button.tsx:112–113,SelectableCard.tsx:79), and the contract checks only durations. - The velocity tracker offers four selectable filters and keeps a non-zero release after a rest (
VelocityTracker.ts:43,VelocityTracker.test.ts:99), which the currentspec:AST-100FR2 andarchitecture:interaction-modalityINV10 rule out. springs.menuReducedMotionshould not be a theme role under DR4, and the new build check also reports an app's own variables such as--xlas undeclared (packages/build/src/vite.ts:411–432).
[Automated review]
| * header describes, `uikit` by default. | ||
| * @experimental | ||
| */ | ||
| export type VelocityStrategy = 'uikit' | 'lsq2' | 'iosFling' | 'window'; |
There was a problem hiding this comment.
spec:AST-100 FR2 and architecture:interaction-modality INV10, current on main, allow one release filter (iOS's blend) and require a release velocity of 0 after the contact stays still longer than the stillness interval. This union adds three alternative filters, PanGestureRecognizer's velocityStrategy option exposes them, and the default "never decays" (line 19). Keeping only the blend, adding the stillness reset and dropping the option would conform.
| }, | ||
| ); | ||
|
|
||
| it('uikit reports the last moving value after a rest, as UIKit does', () => { |
There was a problem hiding this comment.
This pins the release spec:AST-100 FR2 rules out: its verification row fails "a non-zero release after a still pause". Once the tracker resets after the stillness interval, this case should expect 0 for a lift after a pause longer than that interval.
| }, [ref, recognizer, manageStyle]); | ||
|
|
||
| useIsomorphicLayoutEffect(() => { | ||
| recognizer.setEnabled(isEnabled); |
There was a problem hiding this comment.
When isEnabled turns false mid-gesture, setEnabled cancels the recognizer (GestureRecognizer.ts:557–573); cancelled is an action state, so the listener above calls the caller's onGesture from inside this layout effect. architecture:react-component-runtime INV2 keeps consumer callbacks out of effects, and INV3 asks a new effect to name the external system it synchronizes.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] The experimental touch animator follows design:reduced-motion. Under Reduce Motion every `animate` and `play` (travel or opacity, on a spring or any curve) becomes an opacity-only cross-fade of 200 ms on a coarse primary pointer, or for a call marked `touchRelease`, and completes at once on any other pointer; nothing moves, scales or springs, and travel lands with a plain style write while the element is transparent, never as an animated transform. `timing.reducedMotionCrossFadeMs` is now 200 ms and is no longer a theme role. The `reducedMotion` and `crossfade` call options are removed; `crossfadeAs` stays. `animator.reducedMotion()` reports the mode for feedback with no fade of its own, `reducedMotionFade()` gives the same fade to code beside the animator, and `reducedMotion.stylex.ts` holds its constants for StyleX transitions. |
There was a problem hiding this comment.
The animator is internal with no experimental subpath, and no release shipped the reducedMotion and crossfade options this note says are removed. Describing it as internal, as the sibling gesture-* and uikit-* changesets do, would keep the release notes accurate.
| "rootDir": "./src", | ||
| "paths": { | ||
| "@astryxdesign/core/motion/*": ["../core/dist/motion/*"], | ||
| "@astryxdesign/core/gestures": ["../core/dist/gestures/index"], |
There was a problem hiding this comment.
No Lab file imports these paths at this head, architecture:interaction-modality INV10 says Lab cannot import the velocity tracker that @astryxdesign/core/gestures exposes, and core's exports map lists none of these subpaths, so a Lab import would not resolve for npm consumers. Dropping this block and its twin in tsconfig.build.json until Lab needs an exported path would avoid that.
There was a problem hiding this comment.
Changes requested: under reduced motion on a touch screen, Button, ClickableCard, SelectableCard and Thumbnail still animate colour, border colour and shadow for 200 ms, while design:reduced-motion DR2 allows only an opacity cross-fade there. Please carry those state changes on opacity (for example by fading the ::after overlay) and have the DR2 browser test assert transition-property as well as duration. The diff against main also carries about 19,800 lines from the stacked predecessors listed in the description (#8045, #8056, #8061, #8067, #8071–#8074, #8258, #8076) and the build fix merged into them (#8075), so please land those first so this pull request shows only its own change. The press-scale and fine-pointer fixes behave as intended in Storybook.
[Automated review]
| // DR4). | ||
| '@media (prefers-reduced-motion: reduce) and (not (pointer: coarse))': | ||
| '0s', | ||
| '@media (prefers-reduced-motion: reduce) and (pointer: coarse)': '200ms', |
There was a problem hiding this comment.
Under (prefers-reduced-motion: reduce) and (pointer: coarse) the border keeps transition-property: border-color (L181) for 200 ms, and the ::after overlay's background-color (L118–119) gets the same 200 ms at L134–135. design:reduced-motion DR2 asks for an opacity-only change on a coarse pointer, and its verification row fails “a property other than opacity”. Carrying the border and overlay change on a layer whose opacity fades under this arm would conform.
| // (motion/reducedMotion.stylex.ts), spelled out because that module is | ||
| // internal and a swizzled component imports only exported paths; | ||
| // motion/reducedMotionValues.test.ts holds them equal. | ||
| '@media (prefers-reduced-motion: reduce) and (pointer: coarse)': |
There was a problem hiding this comment.
As in ClickableCard, this arm gives the overlay's background-color transition (L221–222) 200 ms on a coarse pointer, where design:reduced-motion DR2 allows only opacity. Fading the overlay by opacity under this arm would conform.
There was a problem hiding this comment.
Changes requested: with reduced motion on a touch screen, Button, ClickableCard, SelectableCard and Thumbnail still animate colour and border colour for 200 ms (selecting a SelectableCard in Storybook runs four 200 ms border-colour transitions), but design:reduced-motion DR2 allows only an opacity cross-fade there, and Button's own record (FR15) keeps its transitions instant under reduced motion. Please carry the cards' touch changes on opacity (for example by fading the ::after overlay or a ring layer), keep Button at 0s under reduced motion on every pointer, and make the DR2 browser test assert transition-property as well as duration. The diff against main also carries about 16,000 lines of stacked work from the predecessors in the description, so please land those first so this pull request shows only its own change.
- In the stack, the gesture host's fine-pointer action-group exception cites a
family:action-groupsrecord that does not exist onmain, while AST-094 FR19 says no recognizer attaches under a fine pointer. - The gestures README says
usePressTrackerrebuilds its tracker when its options change, but the hook builds it once from the first render's options.
[Automated review]
| '@media (prefers-reduced-motion: reduce) and (pointer: coarse)': | ||
| 'background-image, background-color, color, opacity', |
There was a problem hiding this comment.
Under reduced motion on a touch screen this arm transitions background-image, background-color, color, opacity for 200 ms. Button's record (FR15) keeps its hover and press transitions instant under reduced motion, and design:reduced-motion DR2 allows only opacity; keeping none / 0s here on every pointer, with the scale(1) arms, conforms to both.
| // (motion/reducedMotion.stylex.ts), spelled out because that module is | ||
| // internal and a swizzled component imports only exported paths; | ||
| // motion/reducedMotionValues.test.ts holds them equal. | ||
| '@media (prefers-reduced-motion: reduce) and (pointer: coarse)': '200ms', |
There was a problem hiding this comment.
On a touch screen with reduced motion, selecting a card now runs four 200 ms border-*-color transitions, because the transition-property above still lists box-shadow, border-color. DR2 allows only an opacity cross-fade here: carry the selection change on a layer whose opacity fades, and list only opacity in the coarse arm.
| // DR4). | ||
| '@media (prefers-reduced-motion: reduce) and (not (pointer: coarse))': | ||
| '0s', | ||
| '@media (prefers-reduced-motion: reduce) and (pointer: coarse)': '200ms', |
There was a problem hiding this comment.
This keeps border-color (and the overlay's background-color) animating for 200 ms on a touch screen under reduced motion; DR2 allows only opacity there, so the coarse arm should fade an opacity layer instead.
| '@media (prefers-reduced-motion: reduce) and (pointer: coarse)': | ||
| '200ms', |
There was a problem hiding this comment.
On a touch screen under reduced motion the overlay's background-color now fades for 200 ms; DR2 allows only opacity, so fade the overlay's opacity instead of its colour.
| test('on a touch screen, a button’s and a card’s colour changes stay cross-fades of 200 ms or less under reduced motion (DR2)', async ({ | ||
| browser, | ||
| }) => { | ||
| const button = await open(browser, PHONE, BUTTON); | ||
| try { | ||
| const {element} = await transitionsOf(button.page, 0); | ||
| expect(element).toBeGreaterThan(0); | ||
| expect(element).toBeLessThanOrEqual(200); | ||
| } finally { | ||
| await button.close(); | ||
| } | ||
| for (const [name, story, index] of CARDS) { | ||
| const card = await open(browser, PHONE, story); | ||
| try { | ||
| const {overlay} = await transitionsOf(card.page, index); | ||
| expect(overlay, name).toBeGreaterThan(0); | ||
| expect(overlay, name).toBeLessThanOrEqual(200); | ||
| } finally { | ||
| await card.close(); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
This DR2 case checks only that the longest duration is above 0 and at most 200 ms, so it passes while Button and the cards transition colour, border colour and shadow. Also read transitionProperty on the element and its ::after and expect opacity alone, and select a SelectableCard to assert which properties actually transition.
| * the touch. | ||
| * - Where the primary pointer is fine the host listens for nothing | ||
| * (`spec:AST-094` FR19, FR24) except an action group | ||
| * (`AttachOptions.actionGroup`, `family:action-groups` FR5): one passive |
There was a problem hiding this comment.
family:action-groups FR5 does not exist on main, and AST-094 FR19 says that where the primary pointer is fine no recognizer attaches and no touch listener is registered. This exception needs a current record that grants it, or should be dropped.
| - `usePressTracker` rebuilds its tracker when `trackingExpansion`, | ||
| `highlightDelay`, `cancelTravel` or `quickTapFlash` change, from the next | ||
| touch, and reads its callbacks (`isBraking` included) on every touch. |
There was a problem hiding this comment.
The hook builds its PressTracker once, in useState, from the first render's options (useGestureRecognizer.ts L136–172), so a later change to trackingExpansion, highlightDelay, cancelTravel or quickTapFlash is ignored. Either rebuild the tracker when they change, or say here that they are read once.
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. It starts once the pull request leaves draft and a maintainer adds the ci:run label.
Under reduced motion, Button no longer scales on a press (component:Button FR15, design:reduced-motion DR1), the cards' colour changes are immediate with a mouse or trackpad (DR3), and on a touch screen only opacity fades within 200 ms, under the DR2 colour exception and DEC-4 approved for this commit; the cards' exit-token touch release follows the release clauses approved with it.
Non-blocking follow-ups:
- The three restated touch-release copies in ClickableCard, SelectableCard and Thumbnail fold into one shared
pressedAlphavariant in #9919, as deferred on this pull request. Button.spec.mdstill says the 0.98 press scale applies under reduced motion (FR15) and that this is listed under Current-state impact; both lines can go now.
[Automated review]
a32fa92 to
a3480a5
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec a3480a5 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. It starts once the pull request leaves draft and a maintainer adds the ci:run label.
Under reduced motion, Button no longer scales on a mouse or touch press (component:Button FR15, design:reduced-motion DR1), and ClickableCard, SelectableCard and Thumbnail change their colours at once (DR3, plus the DR2 colour exception and DEC-4 an engineering owner approved for this head). The cards' touch release on the exit tokens follows the approved ClickableCard FR5, SelectableCard FR4 and Thumbnail FR4 clauses.
Non-blocking, for the next rebase:
- Regression: on
mainthe press controller now waits for a release animation that is still running, so the cards' comment "The controller ends the press on the shared clock, so the clock caps the fade" no longer holds there; drop the cap or restate why it stays. Button.spec.mdL47 still lists the press scale under reduced motion as a conflict with DR1, which this pull request fixes.
[Automated review]
| // The controller ends the press on the shared clock, so the clock | ||
| // caps the fade. |
There was a problem hiding this comment.
Non-blocking: on main, endAfterRelease in utils/pressFeedback.ts waits for a release animation that is still running when the clock ends, so after a rebase the controller no longer cuts a longer fade short. Drop the cap, or say why it stays. The same comment is in SelectableCard.tsx and Thumbnail.tsx.
a3480a5 to
a03df9c
Compare
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
This pull request needs an engineering owner's decision before it can be approved.
Under reduced motion, Button no longer scales on a press (component:Button FR15, design:reduced-motion DR1), and the three cards' colour changes become immediate with a mouse (DR3). The rest rests on record changes this pull request proposes: the design:reduced-motion DR2 colour exception and DEC-4, and the touch-release clause in component:ClickableCard FR5, component:SelectableCard FR4 and component:Thumbnail FR4. The approval given for the previous head does not apply after the rebase, so an engineering owner needs to record the decision by commenting /approve-spec <full head SHA> on this pull request, or reject any of those changes; the author may do so as an engineering owner.
Non-blocking:
- The comment "The controller ends the press on the shared clock, so the clock caps the fade" in the three cards no longer holds: after the rebase the press controller waits for a running release animation, so keep only the reason for the cap or drop it.
Button.spec.mdstill lists the press scale under reduced motion as a conflict with DR1 in its readiness row.
[Automated review]
| // The controller ends the press on the shared clock, so the clock | ||
| // caps the fade. |
There was a problem hiding this comment.
Non-blocking: after the rebase, endAfterRelease in utils/pressFeedback.ts waits for a release animation that is still running when the clock fires, so the controller no longer ends the press on the shared clock. Keep only the reason, e.g. "Cap the release at the shared press clock until that clock follows the exit tokens." The same comment is in SelectableCard.tsx and Thumbnail.tsx.
There was a problem hiding this comment.
Done in d03a931: the three cards now keep only the reason, "Cap the release at the shared press clock until that clock follows the exit tokens."
|
/approve-spec a03df9c |
github-actions
Bot
removed
the
needs:spec-owner-review
a03df9c to
d03a931
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec d03a931 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. It starts once the pull request leaves draft and a maintainer adds the ci:run label.
Under reduced motion, Button no longer scales on a press (component:Button FR15, design:reduced-motion DR1), and ClickableCard, SelectableCard and Thumbnail change their colours at once (DR3, and the DR2 colour exception and DEC-4 approved for this head). On a phone, the three cards' tap release follows the theme's exit duration and curve, as their approved record clauses state.
Non-blocking:
- The description quotes DR2 as "keeps its fade, on the exit tokens within 200 ms"; the record says "keeps its fade of 200 ms or less".
- The three restated release copies are due to fold into the shared press module in #9919 once #9890 lands, as planned.
[Automated review]
|
/approve-spec c377f91 |
There was a problem hiding this comment.
Changes requested. An engineering owner approved this head's record changes, and the reduced-motion fixes behave as described in Chromium: a pressed Button stays full size, and the cards' colours change at once with only opacity fading on a touch screen. Three regressions from #9890 landing on main remain:
- Regression: since #9890, every press release fades on
--duration-exitand--ease-exiton every device and pointer (spec:AST-094FR5), so the cards'touchReleasestyles (ClickableCard.tsxL79–113,SelectableCard.tsxL77–111,Thumbnail.tsxL145–179), their record clauses and the browser spec's 200 ms fine-pointer case now turn a finger's release on a touchscreen laptop back to 200 ms on the old curve and cap a longer theme exit duration; rebase and delete them, sincepressedAlphaalready gives the cards that release. - Regression:
.changeset/cards-touch-release-exit-tokens.mdannounces that the cards' release now fades on the exit tokens, whichmainalready ships, so delete it with the code above. - Regression: the branch no longer merges, with conflicts in
Button.spec.md(keepmain's FR15 release sentence and remove only the press-scale gap note) andscripts/build-css.test.mjs(takemain's release-clock test).
The fix commit needs a fresh /approve-spec <full head SHA> from an engineering owner, since a new commit ends this head's approval.
[Automated review]
| * curve. SYNC: utils/interactionOverlay.stylex.ts PRESS_RELEASE_DURATION, | ||
| * PRESS_RELEASE_DURATION_REDUCED and RELEASE_EASE. | ||
| */ | ||
| const SHARED_RELEASE = `calc(${pressConsts.releaseMs}ms * min(var(--_astryx-motion-tempo, 1), 1.5))`; |
There was a problem hiding this comment.
Regression: since #9890 landed on main, the shared release in utils/interactionOverlay.stylex.ts is --duration-exit on --ease-exit for every pointer (spec:AST-094 FR5). This restated 200ms × tempo on cubic-bezier(0, 0, 0.2, 1), composed after pressedAlpha, turns a finger's or pen's release on a touchscreen laptop back to 200 ms on the old curve, and the coarse arm caps a longer theme exit duration. Delete touchRelease and these constants; pressedAlpha already gives the card the exit-token release.
There was a problem hiding this comment.
Done in 0fcf4c0: touchRelease and the SHARED_RELEASE* constants are gone, and the card composes pressedAlpha alone.
| * curve. SYNC: utils/interactionOverlay.stylex.ts PRESS_RELEASE_DURATION, | ||
| * PRESS_RELEASE_DURATION_REDUCED and RELEASE_EASE. | ||
| */ | ||
| const SHARED_RELEASE = `calc(${pressConsts.releaseMs}ms * min(var(--_astryx-motion-tempo, 1), 1.5))`; |
There was a problem hiding this comment.
Regression: same as in ClickableCard: on main this restated release overrides the exit-token release spec:AST-094 FR5 requires on every pointer. Delete touchRelease and these constants.
There was a problem hiding this comment.
Done in 0fcf4c0: touchRelease and the SHARED_RELEASE* constants are gone, and the card composes pressedAlpha alone.
| * curve. SYNC: utils/interactionOverlay.stylex.ts PRESS_RELEASE_DURATION, | ||
| * PRESS_RELEASE_DURATION_REDUCED and RELEASE_EASE. | ||
| */ | ||
| const SHARED_RELEASE = `calc(${pressConsts.releaseMs}ms * min(var(--_astryx-motion-tempo, 1), 1.5))`; |
There was a problem hiding this comment.
Regression: same as in ClickableCard: on main this restated release overrides the exit-token release spec:AST-094 FR5 requires on every pointer. Delete touchRelease and these constants.
There was a problem hiding this comment.
Done in 0fcf4c0: touchRelease and the SHARED_RELEASE* constants are gone, and Thumbnail composes pressedAlpha alone.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] Under a coarse primary pointer, the press release of ClickableCard, SelectableCard and Thumbnail now fades on the theme's exit duration and easing, capped at the press's release time. |
There was a problem hiding this comment.
Regression: main already fades these cards' release on the exit tokens, on every pointer (#9890's changeset names cards). This entry would announce a change that is not this pull request's; delete it with the card release styles.
There was a problem hiding this comment.
Done in 0fcf4c0: the changeset is deleted.
| } finally { | ||
| await card.page.mouse.up(); | ||
| } | ||
| // A finger's release on this desktop keeps the shared clock and curve. |
There was a problem hiding this comment.
Regression: this pins a finger's release on a desktop at 200 ms on cubic-bezier(0, 0, 0.2, 1); on main, spec:AST-094 FR5 makes it --duration-exit on --ease-exit. Once the card release styles go, expect the exit tokens here (as the touch-screen case above does).
There was a problem hiding this comment.
Done in 0fcf4c0: the fine-pointer case expects a finger's release on the exit tokens.
|
/approve-spec 0fcf4c0 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. It runs once the pull request leaves draft and a maintainer adds the ci:run label, since the branch is in a fork.
Under reduced motion, a pressed Button now stays full size and the three cards change their colours at once, with only opacity fading on a touch screen within 200 ms; an engineering owner approved the record changes for this head, and the earlier findings are fixed.
- Optional wording fix: "A press shows at once" in the ClickableCard, SelectableCard and Thumbnail clauses also reads as covering a mouse press, which still fades in over
--duration-fast; "A touch press shows at once" would match the code.
[Automated review]
| | FR2 | A primary-button press on the card surface calls `onClick` once with the card as `currentTarget`, then performs an accepted navigation unless the callback cancels it. A middle press on the surface of a card given `href` opens an accepted destination in a new tab without calling `onClick`. A press inside a nested interactive descendant, inside an open Tooltip or HoverCard surface (`component:Tooltip` FR11), or one that ends a text selection, does not activate the card. Activating the hidden button of an action card calls `onClick` once; activating the hidden link navigates without calling `onClick`. | Current source and tests; `architecture:public-component-api` INV7 | | ||
| | FR3 | Plain, new-tab, modified, middle-click, and surface-delegated navigation use the shared destination decision; a blocked destination does not navigate. | `family:navigation-destinations` FR1, FR3–FR5 | | ||
| | FR4 | `isDisabled` keeps the hidden control focusable with `aria-disabled="true"`; Enter, Space, and clicks on it do nothing. A disabled link keeps its link role and name but has no `href` or `target`. The control stays focusable and announced (`spec:AST-085` FR1, FR2, FR5); the card's own click and mouseup handling and the caller's `onMouseUp` are removed, other caller event props still reach the root, and the surface shows no hover or pressed paint (`spec:AST-085` FR6), and renders at half opacity. The disabled state is reflected on the owning target (`architecture:component-theming-surface` INV6). Implementation gap: the `clickable-card` target reflects no disabled state (Current-state impact). | Current source and tests | | ||
| | FR5 | While enabled, the root paints the shared hover overlay under a hover-capable pointer and the pressed overlay for mouse and touch presses that would activate the card; a touch press that starts on a nested interactive element shows no pressed paint, since it does not activate the card (the counterpart of FR2's activation rule), while a mouse press there keeps the `:active` paint, and a bordered card's border responds to hover. A press shows at once. Under the touch presentation, its release fades on the theme's exit motion tokens (`--duration-exit`, `--ease-exit`), per `spec:AST-094` FR5; a mouse press keeps the card's desktop press styling. Keyboard focus on the hidden control shows the shared focus outline on the root. | Current source, stories, and `architecture:interaction-modality` | |
There was a problem hiding this comment.
Optional: "A press shows at once" also reads as covering a mouse press, which still fades in over --duration-fast (the browser spec's desktop case asserts that timing while the mouse is down). "A touch press shows at once" matches the code; the same sentence is in SelectableCard FR4 and Thumbnail FR4.
There was a problem hiding this comment.
Done in ab1d195: the clause reads "A touch press shows at once" in the three card records.
Under reduced motion a press never scales a Button, and ClickableCard, SelectableCard and Thumbnail change their hover and selection colors at once on every pointer. The three cards drop their own copy of the press release and compose the shared pressedAlpha alone. Under the touch presentation their press release fades on the theme's exit duration and easing, as other touch press releases do (component:ClickableCard FR5, component:SelectableCard FR4, component:Thumbnail FR4). The press still shows at once. Under reduced motion that fade takes at most 200 ms, opacity only. A mouse press keeps the cards' desktop press styling.
|
/approve-spec ab1d195 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. It runs once the pull request leaves draft and a maintainer adds the ci:run label, since the branch is in a fork.
Under reduced motion, a pressed Button now stays full size, and ClickableCard, SelectableCard and Thumbnail change their hover, press, border and selection colours at once; on a touch screen only opacity still fades, within 200 ms. An engineering owner approved this head's record changes, real-browser checks show the new behaviour against the base, and nothing blocks.
- Optional follow-up: the card clauses keep a mouse press on the cards' fast CSS fade, which the touch-presentations spec's press rule (FR5, and DEC-21's "instead of the browser's
:active") does not carve out; this is already howmainbehaves, so aligning the two is a separate decision.
[Automated review]
User impact
For people who turn on reduced motion, a pressed Button still shrinks to 98%, which the setting asks it not to do. Button's colors already change at once under reduced motion, but the cards' don't: ClickableCard, SelectableCard and Thumbnail still fade their hover, press and selection colors, border and ring included, on every pointer.
The cards' press release doesn't change. They compose the shared press release, which fades on the theme's exit tokens on every pointer (
spec:AST-094FR5), and their records now state that release.Problem and solution fit
design:reduced-motionbecame current. Checking Button and the cards against it found that Button's press scale has no reduced-motion rule, and the cards keep their color, border and shadow transitions on every pointer.Under reduced motion, Button stays full size. The cards change their colors at once with a mouse or a trackpad. On a touch screen their colors change at once too, and only opacity may still fade, in 200 ms or less: a finger's press highlight keeps its release fade.
Expected behavior and authority
design:reduced-motionDR1: a press never scales a button.component:ButtonFR15 says the same for Button. This pull request removes FR15's gap note and the matching Current-state bullet, and names the browser case in FR15's verification row. Button's loading Spinner is out of scope here.design:reduced-motionDR3: with a fine primary pointer, a change is immediate.design:reduced-motionDR2, the touch screen: only opacity fades, in 200 ms or less, whatever the theme sets (DR4).component:ClickableCardFR5,component:SelectableCardFR4 andcomponent:ThumbnailFR4 each add the spec owner's wording (2026-11-04): "A touch press shows at once. Under the touch presentation, its release fades on the theme's exit motion tokens (--duration-exit,--ease-exit), perspec:AST-094FR5; a mouse press keeps the card's desktop press styling." Each record's Governing rules addspec:AST-094/FR5, and its Compatibility line cites the clause.Card 31: (A), decided by the spec owner cixzhang on 2026-11-03.
design:reduced-motionsays, beside DR2's in-place-value line: "A control's hover or selection color, such as SelectableCard's selection border and ring, also applies at once on any pointer, as on iOS (DEC-4). So does the onset of its press color. A color that changes with a part that cross-fades follows that cross-fade, such as Switch's track with its thumb (component:SwitchFR15). A touch press's release keeps its fade of 200 ms or less." DEC-4 records the decision (rejected: a cross-fade of a hover or selection color on a touch screen). The Switch sentence is the companion clause from the owner's motion rule (2026-11-04), and DEC-4's decider line says so. The user-intent line, the coarse-pointer State row and a new "DR2 colors" verification row carry the same exception, andverified_bynames the browser spec.The records are owner-gated, so they need an engineering owner's
/approve-spec <head sha>.Desktop before and after
--duration-exitand--ease-exit, as on main. Its frames match card 26's preview (below).Smallest restoration
--ease-exitovermin(var(--duration-exit), 200ms), a disabled card's dimming included.reducedMotionValues.test.tskeeps each copy equal toreducedMotionFadeConsts.duration, and reads themin(…, 200ms)spelling.pressedAlphaalone, as on main. Since fix(core): a press's release fades on the theme's exit motion tokens #9890 it releases on--duration-exitand--ease-exit, withmin(--duration-exit, 200ms)under reduced motion, so the cards keep no copy of it.Tradeoffs: the 200 ms bound is copied into the components, held by the test above. No API change.
Rebase
On main 9d068fc, which includes #9890 (7f8de29). It merges cleanly into main 4e7823a. By the spec owner's decision (2026-11-04), this commit also carries #9919's fold, and #9919 is closed. Against main:
SHARED_RELEASE*,touchRelease), their sync testsharedPressRelease.test.tsand the cards changeset are gone.scripts/build-css.test.mjs, the three card unit tests and__tests__/pressState.tsequal main's.Button.spec.mdFR15 is main's exit-token clause without the old gap sentence about the 0.98 press scale.ReducedMotionPress.a11y.chromium.spec.ts, the fine-pointer case expects a finger's release on the exit tokens.Overlaps with open pull requests
Textual conflicts with this head, in its files: #9783 (
Button.spec.md), #9930 (the ClickableCard and SelectableCard records) and #9097 (SelectableCard.tsx). #8872, #8874 and #9997 edit the same files and merge cleanly. Whichever merges second rebases.Writing rubric results
Run over the five changed records:
design:reduced-motion,component:Button,component:ClickableCard,component:SelectableCardandcomponent:Thumbnail.proposes,we decided,previously,a port ofandnowover the added lines finds nothing. The new FR clauses, Compatibility lines and Builder-impact cells are present tense, and the stale "The record describes shipped behavior" lines are gone from the three card records.design:reduced-motionhas four, each oneRejected:line inside its decision (DEC-1 to DEC-4). The Button and card records have none; Button's one "rejected" is behavior text ("A rejected destination renders inertly").#followed by digits,/pull/or/issues/).component:ClickableCardOQ1 is unanswered and keeps its two options.component:ButtonOQ1 (checkable) is unanswered and names the receipt that settles it. The other three records have none.Scope
Testing
To check by hand: in Storybook, turn on reduced motion (the OS setting, or DevTools' "Emulate prefers-reduced-motion"). Press a Button with the mouse: it doesn't shrink, and its color changes at once. With touch emulation, tap a card: only its overlay's press fades out, within 0.2 s. Selecting a SelectableCard changes its border and ring at once. With reduced motion off and touch emulation, tap a card: its press shows at once and fades out on the theme's exit timing, as on main. With a mouse, a card's hover and press look as on main.
Local gates at 0fcf4c0, on main 9d068fc, after a fresh
pnpm install --frozen-lockfile(exit 0). This head differs from 0fcf4c0 only in the three card records, where the clause reads "A touch press shows at once" (the spec owner's correction, 2026-11-04), so its build is identical and the evidence below carries. At this head,check:knowledge, Prettier,CI=true pnpm linton the changed records andreview:prepass.ReducedMotionPress.a11y.chromium.spec.ts, run on a Storybook built at main 9d068fc, fails 4 of 6 cases: the Button press scale under reduced motion (DR1), immediate changes with a fine pointer (DR3), and the two touch-screen reduced-motion cases (opacity-only fades, and SelectableCard's selection). The two touch-release cases pass on main, where the cards' release is already the shared one.ReducedMotionPress.a11y.chromium.spec.ts6/6;CardPress.a11y.chromium.spec.ts9/9; Button's accessibility contract 20/20.test:ui20,275/20,275;test:node16,514 passed, 2 expected failures, none failed.pnpm build, typecheck,CI=true pnpm linton the changed files, strict ESLint, Prettier,check:knowledgeandreview:prepass.Card 26's preview against this head
Local probe, not part of the diff. Card 26's preview (built from a03df9c) was mirrored from its deployment. 601 of its 608 files are byte-identical to a local build of a03df9c, including every script and stylesheet. The other 7 are the iframe page (the host adds a
noindexmeta tag), the story index, the project file and three Storybook manager bundles.The probe served the mirrored files and this head's build side by side, in real Chromium with phone emulation (402×714, touch, coarse primary pointer), and made one real tap on each card through touch input. When the press controller writes the release, the probe pauses the release animation and seeks it to each time below. At each time it reads the surface's
--astryx-press-alpha, the overlay's computed paint, and a screenshot of the card.Desktop against main
Local probe, not part of the diff. Desktop hover and mouse press on the three cards, compared with main's own Storybook build at 9d068fc, at 1024×768 with a fine pointer.