Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
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.
82e3e8e to
072d91d
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved, and five fixes are needed.
At this head the design:reduced-motion DR2 colour exception and DEC-4 have no owner approval. No current record governs the cards' touch release on --duration-exit and --ease-exit either: spec:AST-094 FR5 only requires compositor motion. An engineering owner records the decision by commenting /approve-spec <full head SHA> and names the clause that governs the exit-token release; the author may comment if they are an engineering owner.
Required fixes:
- On a phone under reduced motion, the card root's opacity transition runs on
--duration-fastbecause this head dropped its 200 ms cap (SelectableCard.tsxL128–133,ClickableCard.tsxL232–237), so a theme can stretch a disabled card's dimming past DR2's 200 ms. - The cards' touch release now runs on
--duration-exitwhile the press controller still clears it after 200 ms × tempo, so with--duration-exit: 400msevery tap's highlight disappears at about two-thirds strength. - The release is copied into three identical
touchReleaseblocks, held equal by a sync test, instead of living as a variant ofpressedAlphaininteractionOverlay.stylex.ts, which the cards already import. - The changeset calls the exit-token release a
[fix], and the description citesspec:AST-094FR5 for it, but main's 200 ms release matched every current record, so describe it as a change. - DR2 and DEC-4 now name "the exit tokens", but the record's own content boundary says it defines no tokens, so keep "within 200 ms" and drop the token name.
Button's reduced-motion scale and the fine-pointer changes look right.
[Automated review]
| '@media (prefers-reduced-motion: reduce) and (pointer: coarse)': | ||
| 'opacity', | ||
| }, | ||
| transitionDuration: { |
There was a problem hiding this comment.
This head dropped the coarse reduced-motion arm here, so on a touch screen the root transitions opacity on --duration-fast. The disabled opacity: 0.5 then fades for as long as the theme sets: 400 ms with --duration-fast: 400ms, past DR2's 200 ms (and DR4). Add back a (prefers-reduced-motion: reduce) and (pointer: coarse) arm of min(var(--duration-exit), 200ms), or 0s.
| '@media (prefers-reduced-motion: reduce) and (pointer: coarse)': | ||
| 'opacity', | ||
| }, | ||
| transitionDuration: { |
There was a problem hiding this comment.
Same as SelectableCard: the bordered root lists opacity on --duration-fast under reduced motion on a touch screen, with no 200 ms cap, so a disabled card's dimming can run 400 ms under a theme.
| '@media (prefers-reduced-motion: reduce) and (not (pointer: coarse))': | ||
| SHARED_RELEASE_REDUCED, | ||
| '@media (pointer: coarse)': { | ||
| default: durationVars['--duration-exit'], |
There was a problem hiding this comment.
The fade now lasts --duration-exit, but pressFadeMs (pressFeedback.ts L156–166) still clears data-astryx-press after 200 ms × tempo. With --duration-exit: 400ms the highlight vanishes at 0.655 strength (measured on all three cards). Cap this arm at the controller's clock, or have the controller read the same duration.
| * 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.
These constants and the touchRelease block below repeat PRESS_RELEASE_DURATION, PRESS_RELEASE_DURATION_REDUCED and RELEASE_EASE from interactionOverlay.stylex.ts, identically in all three cards. Exporting the coarse-pointer release as a pressedAlpha variant there would give each card one style to compose, and would retire sharedPressRelease.test.ts.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Under a coarse primary pointer, ClickableCard, SelectableCard and Thumbnail fade a press's release on the theme's exit duration and easing. Under reduced motion, a pressed Button no longer shrinks, and the three cards change their hover, border and selection colors at once on every pointer, whatever the theme's duration. A finger's press highlight still fades out on release, on the exit tokens within 200 ms. |
There was a problem hiding this comment.
The exit-token release is a behaviour change, not a fix: main's 200 ms release matches every current record. Suggested: "Under a coarse primary pointer, the three cards' press release now fades on the theme's exit duration and easing."
| onset of its press color. A touch press's release keeps its fade, on the | ||
| exit tokens within 200 ms. |
There was a problem hiding this comment.
The Content boundary below says this record "does not define … tokens". Keep "within 200 ms" here and in DEC-4, drop "on the exit tokens", and let the record that governs the release name its duration.
072d91d to
d6b4c4b
Compare
|
/approve-spec d6b4c4b |
There was a problem hiding this comment.
Changes requested. The reduced-motion fixes behave as intended, and an engineering owner approved the record changes at this head; three fixes remain.
- The capped exit-token release (
SHARED_RELEASE*andtouchRelease) is copied intoClickableCard.tsx,SelectableCard.tsxandThumbnail.tsx; add it as apressedAlphavariant inutils/interactionOverlay.stylex.ts, compose that in each card, and drop the copies andsharedPressRelease.test.ts. - The three card records cite "the spec owner's press-fade ruling (2026-11-04)" under Governing rules and Compatibility, which no record contains; state the release as a clause of each record (for example in FR5), cite that clause, and drop the stale "no API, default, or output changes" from the ClickableCard and Thumbnail summaries.
- Miss: the description has no
## Writing rubric resultssection although four records change; add the R1–R6 results thatdocs/contributing/spec-writing.mdasks for.
Two wording fixes would also help: the [fix] changeset's "on the exit tokens" (the reduced-motion release ran on the shared 200 ms clock before) and the commit message's spec:AST-094 FR5 citation.
[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.
These constants and the touchRelease block below are repeated verbatim in SelectableCard.tsx and Thumbnail.tsx, and restate PRESS_RELEASE_DURATION, PRESS_RELEASE_DURATION_REDUCED and RELEASE_EASE from utils/interactionOverlay.stylex.ts. That module already owns the release (pressReleaseAnimation) and every card composes its pressedAlpha, so a pressedAlpha variant there with the capped exit-token arm would let each card make one call and remove the three copies and sharedPressRelease.test.ts.
There was a problem hiding this comment.
Kept as three copies by the spec owner's decision: they fold into one pressedAlpha variant in utils/interactionOverlay.stylex.ts after #9890 merges, so the shared press module isn't edited by two pull requests at once.
There was a problem hiding this comment.
Fair enough — deferring the consolidation to avoid editing the shared press module in two PRs at once is a reasonable sequencing call, and the sync test guards the three copies against drifting in the meantime.
I'll take this as intentionally deferred rather than addressed: the next full review will re-evaluate it, including whether #9890 has merged and the fold into a single pressedAlpha variant in utils/interactionOverlay.stylex.ts is now unblocked. If #9890 slips, it'd be worth revisiting rather than letting the three copies linger indefinitely.
| | Builder impact | None; the record describes shipped behavior. | | ||
| | Compatibility/readiness | Released behavior recorded as current; no API, default, or output changes. Two implementation gaps are listed under Current-state impact. Under a coarse primary pointer the press release fades on the theme's exit tokens, by the spec owner's press-fade ruling (2026-11-04). | | ||
| | Review checks | Reject a live destination while disabled, duplicate activation, activation from a nested control or selected text, a destination that skips the shared decision, or lost focus paint. | | ||
| | Governing rules | `family:navigation-destinations/FR1,FR3–FR5`; `architecture:interaction-modality/INV2–INV5`; `architecture:public-component-api/INV5–INV8`; `spec:AST-085` FR1, FR2, FR5, FR6; the spec owner's press-fade ruling (2026-11-04). | |
There was a problem hiding this comment.
Governing rules should link only current record clauses (architecture:knowledge-contracts), and "the spec owner's press-fade ruling (2026-11-04)" is not in any record. Stating the release as a clause here (for example in FR5: under a coarse primary pointer the touch release fades on --duration-exit and --ease-exit, capped at the press controller's clock) and citing that clause would fix this row, the Compatibility bullet and the same lines in SelectableCard and Thumbnail.
There was a problem hiding this comment.
Done in f21b77c: the release is now a clause of ClickableCard FR5, SelectableCard FR4 and Thumbnail FR4, which Governing rules and Compatibility cite.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Under reduced motion, a pressed Button no longer shrinks, and the three cards change their hover, border and selection colors at once on every pointer, whatever the theme's duration. A finger's press highlight still fades out on release, on the exit tokens within 200 ms. |
There was a problem hiding this comment.
Non-blocking: before this change the reduced-motion release ran on the shared 200 ms clock, not the exit tokens, and the [feat] changeset already announces the tokens. Suggested ending: "A finger's press highlight still fades out on release, within 200 ms."
d6b4c4b to
b80e5a1
Compare
b80e5a1 to
f21b77c
Compare
|
/approve-spec f21b77c |
There was a problem hiding this comment.
Changes requested. The reduced-motion fixes behave as intended, an engineering owner approved the record changes at this head, and the earlier record-citation and rubric fixes are done; one fix remains.
- The capped exit-token release (
SHARED_RELEASE*andtouchRelease) is still copied intoClickableCard.tsx,SelectableCard.tsxandThumbnail.tsx, so add it as apressedAlphavariant inutils/interactionOverlay.stylex.ts, compose that in each card and drop the copies andsharedPressRelease.test.ts, or have an engineering owner record on this pull request that the fold waits for #9890.
A few record touch-ups would also help: the card records' "No API or default changes" beside "Released default preserved: no", the press-scale gap still listed in component:Button FR15, and the cards' verified_by lists, which omit the new browser spec.
[Automated review]
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. Under a coarse primary pointer the three cards' press release fades on the theme's exit duration and easing, capped at the press controller's release clock (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 fine primary pointer keeps the timing it had for hover and a mouse press.
f21b77c to
a32fa92
Compare
|
/approve-spec a32fa92 |
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]
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.
This pull request also changes, rather than fixes, one behavior: under a coarse primary pointer, the three cards' touch press release fades on the theme's exit tokens instead of the press controller's own clock and curve (ClickableCard FR5, SelectableCard FR4, Thumbnail FR4).
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. 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 adding the spec owner's press-fade ruling (2026-11-04) as the clause "Under a coarse primary pointer, the touch press shows at once and its release fades on the theme's exit motion tokens (--duration-exit,--ease-exit); until the shared press clock follows those tokens, the release is capped at it." A fine primary pointer keeps the shared release. Each record's Governing rules and Compatibility cite that clause. The cross-component sentence inspec:AST-094FR5 comes with fix(core): a press's release fades on the theme's exit motion tokens #9890.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 touch press's release keeps its fade, on the exit tokens within 200 ms." DEC-4 records the decision (rejected: a cross-fade of a hover or selection color on a touch screen). 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 record is owner-gated, so it needs an engineering owner's
/approve-spec <head sha>.Desktop before and after
cubic-bezier(0, 0, 0.2, 1), at most 200 ms under reduced motion.--duration-exit, at most the controller's clock, on--ease-exit. Under reduced motion it takes at most 200 ms, opacity only.Smallest restoration
--ease-exitovermin(var(--duration-exit), 200ms), a disabled card's dimming included.touchReleasestyle right after the sharedpressedAlpha. Under(pointer: coarse)it sets the release's curve to--ease-exitand its clock to--duration-exit, capped at the press controller's own clock (min(var(--duration-exit), <shared release clock>), and the shared reduced clock under reduced motion, which is at most 200 ms).touchReleaserestates the shared fine-pointer clock and curve.sharedPressRelease.test.tskeeps the restatements equal tointeractionOverlay.stylex.ts.reducedMotionValues.test.tskeeps each copy equal toreducedMotionFadeConsts.duration, and now also reads the cappedmin(…, 200ms)spelling.scripts/build-css.test.mjsholds every shipped release duration to the controller's clock, so it now names the cards' exit-token arms: one per reduced-motion side, eachmin(var(--duration-exit), …)capped at that clock.Tradeoffs: the three cards each restate the shared release, held by those tests. By the spec owner's decision, the copies stay here, and one
pressedAlphavariant inutils/interactionOverlay.stylex.tsreplaces them in a follow-up after #9890 merges, so the shared press module isn't edited by two pull requests at once. The press controller ends the press on the shared clock, so that clock caps each card's fade, and a theme whose--duration-exitis longer gets the shared clock instead. #9890 moves that timer onto the token, and the cap goes then. No API change.Rebase
On main 217fc46. The touch fix needs
--duration-exit, which the earlier base 8925571 predates, so #8326 is replayed onto main with a clean cherry-pick. Main's #9811 makes browser specs import from@astryxdesign/a11y-spec/playwright, so the new spec does. The replayed commit before the fix is banked as NOT-FOR-LANDING evidence (bank/C01/8326-replay-red, 0db8dea) for the red-first below.Overlaps with open pull requests
Lane 2's press-token work (#9762, #9783) moves every other component's press release onto the exit tokens and leaves these three cards to this pull request, so their hunks don't overlap. #8727 edits Button.tsx, and #8872 and #8874 edit the cards' rings; whichever lands second rebases.
Writing rubric results
Run over the four changed records:
design:reduced-motion,component:ClickableCard,component:SelectableCardandcomponent:Thumbnail.proposes,we decided,previously,a port ofandnowfinds 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 card records have none.#followed by digits,/pull/or/issues/).component:ClickableCardOQ1 is unanswered and keeps its two options. 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. With a mouse, a card's hover and press look as on main.
Local gates at a9755c4 (this head differs only in record text, changeset wording and the commit message), on main 217fc46, after a fresh
pnpm install --frozen-lockfile(exit 0):sharedPressRelease.test.ts's guard.ReducedMotionPress.a11y.chromium.spec.ts, Chromium, on a Storybook built at this head): 6/6. Button's accessibility contract: 20/20.test:ui19,743/19,743;test:node16,276 passed, 0 failed.check:knowledgeandreview:prepass.CI=true pnpm lintshows one error, in main's ownDateInput/__tests__/WheelContrast.a11y.chromium.spec.ts, and none in this pull request's files.