Repository navigation
feat(core): a theme's motion takes tempo and character - #8661
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:design-review
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:spec-owner-review
aeb37c1 to
50323d9
Compare
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 needs an engineering owner's decision before it can be approved. The motion knobs rest on spec:AST-108 and the spec:AST-094, spec:AST-090, design:reduced-motion DR2a, design:motion DR3 and architecture:theme-authoring-contract INV15 text this pull request adds, which is not on main and not approved for this head. An engineering owner can approve it by commenting /approve-spec <full head SHA> here (the author can, as an engineering owner); the approval should cover the build's 'crisp' and 'expressive' curves and INV15's rule that a lineage's explicit tokens win, and without it this waits for #8613 to land. Either way, these need fixing:
reducedMotion: 'always'adds the mark, but nothing reads it, so motion is not reduced, although the docs, template and changeset say it is.- A child theme with its own duration scale now loses to its base's explicit
--duration-*and--ease-*tokens, which changes existing themes. - A theme nested inside a
tempotheme times Spinner and Skeleton from the outer theme's pre-tempo values. - The docs say the knobs drive touch springs;
resolveThemeMotiondisagrees with the CSS for an explicit--ease-standard; and Spinner's and Skeleton's records still say the loops take the scaled tokens.
[Automated review]
| const lineageMotionTokens = | ||
| input.motion != null && base?.__inputTokens != null | ||
| ? Object.fromEntries( | ||
| Object.entries(base.__inputTokens).filter( | ||
| ([name]) => | ||
| name.startsWith('--duration-') || name.startsWith('--ease-'), | ||
| ), | ||
| ) | ||
| : undefined; |
There was a problem hiding this comment.
Re-applying every explicit --duration-*/--ease-* token in the lineage whenever the child sets motion changes existing themes. A child with its own scale and easing ({fast: 100, medium: 250, ratio: 0.75, easing: 'ease-in'}) over a base with explicit --duration-medium: 300ms and --ease-standard: linear now resolves to 300 ms and linear instead of its own 250 ms and ease-in. Limiting the re-application to tokens the knobs regenerate keeps today's order: base, then the child's generated values, then explicit tokens.
| if (knobs.tempoIsSet) { | ||
| Object.assign(tokens, durationTokensAt(knobs.tempo)); | ||
| for (const [name, value] of Object.entries(durationDefaults)) { | ||
| tokens[preTempoName(name)] = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
Only a theme that sets tempo writes the --pre-tempo-* copies. A theme nested inside a tempo theme that sets its own durations (a scale or explicit tokens) therefore inherits the outer copies, and its Spinner and Skeleton loops run at 0.73 s and 0.55 s instead of its own 0.6 s and 0.335 s. Writing the copy beside every --duration-* token a theme writes stops the leak.
| items: [ | ||
| "`tempo` scales every motion duration: 1 is Astryx's own, 0.75 the quickest and 1.5 the slowest (values outside are clamped).", | ||
| "`character` sets how lively motion is: `'crisp'` decelerates without overshoot, `'standard'` is Astryx's own, and `'expressive'` overshoots slightly on entrances and moves. Exits and fades never overshoot.", | ||
| "`reducedMotion: 'always'` reduces motion for everyone, as if each person had turned on Reduce Motion; `'system'`, the default, follows each person's setting. No value loosens Reduce Motion.", |
There was a problem hiding this comment.
Nothing reads data-astryx-reduced-motion at this head; only Theme.tsx writes it. With 'always', Spinner keeps turning at its normal speed and Skeleton keeps pulsing, exactly as without it. Until components key on the mark, this sentence and the matching template, JSDoc and changeset wording promise a reduction the release will not have.
| content: [ | ||
| { | ||
| type: 'prose', | ||
| text: "A theme sets how motion feels with three knobs in `defineTheme`'s `motion`. They drive the CSS motion tokens and the springs of touch motion from the same values, so both stay in step.", |
There was a problem hiding this comment.
No animator code reads the knobs yet (resolveThemeMotion has no caller), and no component reads --ease-entrance or --ease-move. In this change the knobs reach neither touch springs nor Astryx's own entrances.
| '--ease-standard': standard, | ||
| '--ease-entrance': value('--ease-entrance', standard), | ||
| '--ease-move': value('--ease-move', standard), |
There was a problem hiding this comment.
For a theme that sets only an explicit --ease-standard, this reports --ease-entrance and --ease-move as that value. expandThemeMotion writes neither token, though, so the CSS keeps the default cubic-bezier(0.24, 1, 0.4, 1). Writing both tokens whenever the theme sets --ease-standard, or returning the CSS default here, keeps the resolver and the CSS in step.
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: the motion knobs rest on spec:AST-108 and the spec:AST-094, spec:AST-090, design:reduced-motion DR2a, design:motion DR3 and architecture:theme-authoring-contract INV15 text this pull request adds, which is not on main and not approved for this head. An engineering owner can approve it by commenting /approve-spec 619b3a5c54cb1b1001bf4257a28fb105fe261592 here (the author can, as an engineering owner), and the approval should cover the build's 'crisp' and 'expressive' curves, the new tempo and character attributes and INV15's inheritance rules; without it this waits for #8613 to land. Either way, these need fixing:
reducedMotion: 'always'adds the mark, but nothing reads it, so motion is not reduced, although the docs, template, JSDoc and changeset say it is.- A child theme that sets its own duration scale now resolves differently: its base's explicit
--duration-*and--ease-*tokens win, and itsratiore-derives an inherited slow band. - A theme nested inside a
tempotheme times Spinner and Skeleton from the outer theme's pre-tempo values. - The docs say the knobs drive touch springs,
--ease-entranceand--ease-moveignore an explicit--ease-standard(whereresolveThemeMotiondisagrees with the CSS), and Spinner's and Skeleton's records still say the loops take the scaled tokens.
[Automated review]
| useRootThemeSync(isNested, mode, theme.name); | ||
| // spec:AST-108 FR6: 'always' marks the theme's root; every component style | ||
| // keyed on prefers-reduced-motion also keys on the mark. FR3: the tempo and | ||
| // character a theme sets ride on the same elements, where the animator |
There was a problem hiding this comment.
Nothing reads data-astryx-motion-tempo or data-astryx-motion-character at this head: no animator or motion code looks for them, and the proposed AST-108 FR6 names resolveThemeMotion as the animator's read. Until FR3 lands they are public DOM output with no effect, and this comment and the JSDoc above useRootThemeSync describe a reader that does not exist yet.
| const baseMotion = base?.__axes?.motion; | ||
| const motion: ThemeMotionConfig | undefined = | ||
| input.motion != null && baseMotion != null | ||
| ? {...baseMotion, ...input.motion} |
There was a problem hiding this comment.
Merging the base's scale fields into a child that sets its own scale changes existing themes too: {fast: 100, medium: 250, ratio: 0.8} over a base with slow: 700, ratio: 0.75 now re-derives the slow band with 0.8, so --duration-slow-min moves from 525 ms to 560 ms (Spinner's cycle) and --duration-slow-max from 935 ms to 875 ms. Merging only the knob fields keeps a child's own scale replacing the base's, as it does on main.
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: the motion knobs rest on spec:AST-108 and the spec:AST-094, spec:AST-090, spec:AST-012, architecture:theme-application INV1, design:reduced-motion DR2a, design:motion DR3 and architecture:theme-authoring-contract INV15 text this pull request adds, which is not on main and not approved for this head. An engineering owner can approve it by commenting /approve-spec d409b49d20b034a517443e7395bf70fdaab4de9e here (the author can, as an engineering owner), and the approval should cover the build's 'crisp' and 'expressive' curves, the tempo and character attributes nothing reads yet and INV15's inheritance rules; without it this waits for #8613 to land. Either way, these need fixing:
reducedMotion: 'always'reaches only adaptation rules, so every component keeps full motion, although the docs, template, JSDoc and changeset say it reduces motion for everyone.- A child theme that sets its own duration scale now resolves differently, since its base's explicit
--duration-*and--ease-*tokens win and itsratiore-derives an inherited slow band, and it resolves differently again over a built base. - A theme nested inside a
tempotheme times Spinner and Skeleton from the outer theme's pre-tempo values. - The docs say the knobs drive touch springs,
--ease-entranceand--ease-moveignore an explicit--ease-standard(whereresolveThemeMotionalso disagrees with the CSS), and Spinner's and Skeleton's records still say the loops take the scaled tokens.
[Automated review]
| input.motion != null && baseMotion != null | ||
| ? {...baseMotion, ...input.motion} | ||
| : input.motion; | ||
| const lineageMotionTokens = |
There was a problem hiding this comment.
Re-applying every explicit --duration-*/--ease-* input token of the base whenever the child sets motion changes existing child themes that set only a duration scale: {fast: 100, medium: 250, ratio: 0.75, easing: 'ease-in'} over a base with --duration-medium: 300ms and --ease-standard: linear resolves to 300 ms and linear here and to 250 ms and ease-in on main. Built theme modules carry no __inputTokens, so the same child resolves differently again over a built base. Limiting the re-application to tokens the knobs regenerate, and applying it the same way over built bases, keeps today's order: base, then the child's generated values, then explicit tokens.
| const baseMotion = base?.__axes?.motion; | ||
| const motion: ThemeMotionConfig | undefined = | ||
| input.motion != null && baseMotion != null | ||
| ? {...baseMotion, ...input.motion} |
There was a problem hiding this comment.
Merging the base's scale fields into a child that sets its own scale re-derives inherited bands with the child's ratio: {fast: 100, medium: 250, ratio: 0.8} over a base with slow: 700, ratio: 0.75 moves --duration-slow-min from 525 ms to 560 ms (Spinner's cycle). Merging only the knob fields keeps a child's own scale replacing the base's, as it does on main.
| ): Record<string, string> { | ||
| const knobs = resolveMotionKnobs(config); | ||
| const tokens: Record<string, string> = {}; | ||
| if (knobs.tempoIsSet) { |
There was a problem hiding this comment.
Only a theme that sets tempo writes the --pre-tempo-* copies, so a theme nested inside a tempo theme that sets its own durations inherits the outer copies: in Chromium its Spinner and Skeleton loops run at 0.73 s and 0.55 s instead of its own 0.6 s and 0.335 s. Writing the copy beside every --duration-* token a theme writes, generated or explicit, stops the leak.
| durations, | ||
| easings: { | ||
| '--ease-standard': standard, | ||
| '--ease-entrance': value('--ease-entrance', standard), |
There was a problem hiding this comment.
For a theme with only an explicit --ease-standard, this reports --ease-entrance and --ease-move as that value, but expandThemeMotion writes neither token, so the CSS keeps cubic-bezier(0.24, 1, 0.4, 1). Given a defined theme, the knobs also come back as tempo 1 and 'standard', because a defined theme keeps them in __axes.motion, not motion.
| items: [ | ||
| "`tempo` scales every motion duration: 1 is Astryx's own, 0.75 the quickest and 1.5 the slowest (values outside are clamped).", | ||
| "`character` sets how lively motion is: `'crisp'` decelerates without overshoot, `'standard'` is Astryx's own, and `'expressive'` overshoots slightly on entrances and moves. Exits and fades never overshoot.", | ||
| "`reducedMotion: 'always'` reduces motion for everyone, as if each person had turned on Reduce Motion; `'system'`, the default, follows each person's setting. No value loosens Reduce Motion.", |
There was a problem hiding this comment.
At this head the mark changes only adaptation rules keyed on when: {motion}; no component style or script reader of prefers-reduced-motion reads it, so under 'always' Spinner keeps turning at its normal speed and Skeleton keeps pulsing for anyone whose own setting is off. Until components key on the mark, this sentence, the template, the JSDoc and the changeset promise a reduction the release does not have.
| content: [ | ||
| { | ||
| type: 'prose', | ||
| text: "A theme sets how motion feels with three knobs in `defineTheme`'s `motion`. They drive the CSS motion tokens and the springs of touch motion from the same values, so both stay in step.", |
There was a problem hiding this comment.
No motion-foundation code reads the knobs yet (resolveThemeMotion has no caller and nothing reads the tempo and character attributes), so in this change the knobs reach neither touch springs nor any Astryx component's entrances.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsSkeleton (@astryxdesign/core) · View in Storybook
Spinner (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: The accessibility audit did not run on this pull request. It runs after merges to Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
…nimator's theme springs
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved. The motion knobs rest on spec:AST-108 and on new AST-094, AST-090, AST-012, theme-application INV1, theme-authoring-contract INV15, reduced-motion DR2a, motion DR3, Spinner and Skeleton text in this pull request; none of it is on main or approved for this head. An engineering owner can approve it by commenting /approve-spec 2a849e7419276c52f7c205ccf49f8fbec0fa28e0 here (the author can, as an engineering owner), and that approval should cover the build's 'crisp' and 'expressive' curves, the tempo and character attributes that nothing reads yet, and INV15's inheritance rule; without it, this waits for #8613 to land. Either way, these need fixing:
reducedMotion: 'always'reaches onlywhen: {motion}adaptation rules, so every component keeps full motion, although the docs, template, JSDoc and changeset say it reduces motion for everyone.- A child theme that sets its own duration scale now resolves differently from
main: its base's explicit--duration-*/--ease-*tokens win, and itsratiore-derives an inherited slow band. It resolves differently again over a built base. - A theme nested inside a
tempotheme times Spinner and Skeleton from the outer theme's pre-tempo values. --ease-entranceand--ease-moveignore an explicit--ease-standard,resolveThemeMotiondisagrees with the CSS, and the docs say the knobs drive touch springs, which nothing reads yet.
[Automated review]
| items: [ | ||
| "`tempo` scales every motion duration: 1 is Astryx's own, 0.75 the quickest and 1.5 the slowest (values outside are clamped).", | ||
| "`character` sets how lively motion is: `'crisp'` decelerates without overshoot, `'standard'` is Astryx's own, and `'expressive'` overshoots slightly on entrances and moves. Exits and fades never overshoot.", | ||
| "`reducedMotion: 'always'` reduces motion for everyone, as if each person had turned on Reduce Motion; `'system'`, the default, follows each person's setting. No value loosens Reduce Motion.", |
There was a problem hiding this comment.
Only Theme.tsx writes data-astryx-reduced-motion at this head, and only when: {motion} adaptation rules read it. With 'always' and the person's own setting off, Spinner keeps its 0.525 s cycle and Skeleton keeps pulsing every 0.4 s, exactly as without the mark. Until component styles and script readers key on the mark, this sentence and the matching template, JSDoc and changeset wording promise a reduction this release does not have.
| const baseMotion = base?.__axes?.motion; | ||
| const motion: ThemeMotionConfig | undefined = | ||
| input.motion != null && baseMotion != null | ||
| ? {...baseMotion, ...input.motion} | ||
| : input.motion; | ||
| const lineageMotionTokens = | ||
| input.motion != null && base?.__inputTokens != null | ||
| ? Object.fromEntries( | ||
| Object.entries(base.__inputTokens).filter( | ||
| ([name]) => | ||
| name.startsWith('--duration-') || name.startsWith('--ease-'), | ||
| ), | ||
| ) | ||
| : undefined; |
There was a problem hiding this comment.
This changes existing child themes that set their own duration scale:
{fast: 100, medium: 250, ratio: 0.75, easing: 'ease-in'}over a base with explicit--duration-medium: 300msand--ease-standard: linearresolves to 300 ms andlinearhere, but to 250 ms andease-inonmain.{fast: 100, medium: 250, ratio: 0.8}over a base withslow: 700, ratio: 0.75moves--duration-slow-minfrom 525 ms to 560 ms.base.__inputTokensis missing from built theme modules, so the first child still gets 250 ms andease-inover a built base.
Inheriting only the knob fields, and re-applying lineage tokens only where the knobs regenerate them, keeps the current order: base, then the child's generated values, then explicit tokens.
| ): Record<string, string> { | ||
| const knobs = resolveMotionKnobs(config); | ||
| const tokens: Record<string, string> = {}; | ||
| if (knobs.tempoIsSet) { |
There was a problem hiding this comment.
Only a theme that sets tempo writes the --pre-tempo-* copies. A theme nested inside a tempo theme that sets its own durations, through a scale or explicit tokens, inherits the outer copies instead. In Chromium its Spinner and Skeleton run at 0.73 s and 0.55 s rather than its own 0.6 s and 0.335 s. Writing the copy beside every --duration-* token a theme writes stops the leak.
| const knobs = resolveMotionKnobs(theme.motion); | ||
| const generated = theme.motion ? expandThemeMotion(theme.motion) : {}; | ||
| const value = (name: string, fallback: string): string => | ||
| theme.tokens?.[name] ?? generated[name] ?? fallback; |
There was a problem hiding this comment.
For a theme whose only motion setting is an explicit --ease-standard: linear, this reports --ease-entrance and --ease-move as linear. expandThemeMotion writes neither token, though, so the CSS keeps cubic-bezier(0.24, 1, 0.4, 1). Given a defined theme, theme.motion is also undefined, because the knobs live in __axes.motion, so the knobs come back as tempo 1 and 'standard'.
| content: [ | ||
| { | ||
| type: 'prose', | ||
| text: "A theme sets how motion feels with three knobs in `defineTheme`'s `motion`. They drive the CSS motion tokens and the springs of touch motion from the same values, so both stay in step.", |
There was a problem hiding this comment.
No animator code reads the knobs yet: resolveThemeMotion has no caller, and nothing reads the tempo and character attributes. In this release the knobs reach neither touch springs nor any Astryx component's entrances.
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: the motion knobs rest on the new spec:AST-108 and on AST-094, AST-090, AST-012, theme-application INV1, theme-authoring-contract INV15, reduced-motion DR2a, motion DR3, Spinner, Skeleton and Button text in this pull request, none of which is on main or approved for this head. An engineering owner can approve it by commenting /approve-spec 5b7ac475ac54f6338d3efa067d7b1139be8670cd here (the author can, as an engineering owner), and that approval should cover the build's 'crisp' and 'expressive' curves, the tempo and character attributes, and INV15's inheritance rule; without it, this waits for #8613 to land. Either way, these need fixing:
reducedMotion: 'always'reaches onlywhen: {motion}adaptation rules, so Spinner and Skeleton keep full motion although the docs, template, JSDoc and changeset say motion is reduced for everyone.- A child theme that sets its own duration scale now resolves differently from
main, since its base's explicit motion tokens win and itsratiore-derives an inherited slow band, and it resolves differently again over a built base. - Under Reduce Motion on a touch screen, a
tempoabove 1 stretches the press release past 200 ms (300 ms at 1.5). - A theme nested in a
tempotheme takes the outer theme's Spinner and Skeleton timing and press tempo, andresolveThemeMotionand the docs about springs and press clocks disagree with what ships.
[Automated review]
| items: [ | ||
| "`tempo` scales every motion duration: 1 is Astryx's own, 0.75 the quickest and 1.5 the slowest (values outside are clamped).", | ||
| "`character` sets how lively motion is: `'crisp'` decelerates without overshoot, `'standard'` is Astryx's own, and `'expressive'` overshoots slightly on entrances and moves. Exits and fades never overshoot.", | ||
| "`reducedMotion: 'always'` reduces motion for everyone, as if each person had turned on Reduce Motion; `'system'`, the default, follows each person's setting. No value loosens Reduce Motion.", |
There was a problem hiding this comment.
Only Theme.tsx writes data-astryx-reduced-motion at this head, and only when: {motion} adaptation rules read it. With 'always' and the person's own setting off, Spinner keeps its 0.525 s cycle and both Skeletons keep pulsing every 0.4 s in Chromium, exactly like a 'system' theme. Until component styles and script readers key on the mark, this sentence and the matching template, JSDoc and changeset wording promise a reduction this release does not have.
| const baseMotion = base?.__axes?.motion; | ||
| const motion: ThemeMotionConfig | undefined = | ||
| input.motion != null && baseMotion != null | ||
| ? {...baseMotion, ...input.motion} |
There was a problem hiding this comment.
This changes existing child themes that set their own duration scale:
{fast: 100, medium: 250, ratio: 0.75, easing: 'ease-in'}over a base with explicit--duration-medium: 300msand--ease-standard: linearresolves to 300 ms andlinearhere, but to 250 ms andease-inonmain.{fast: 100, medium: 250, ratio: 0.8}over a base withslow: 700, ratio: 0.75moves--duration-slow-min, Spinner's cycle, from 525 ms to 560 ms.- Built theme modules carry no
__inputTokens, so the first child keepsmain's result over a built base.
Inheriting only the knob fields (and the base's scale fields only when the child sets none), and re-applying lineage tokens only to names the knobs regenerate, keeps the current order: base, then the child's generated values, then explicit tokens.
| * its duration tokens (`spec:AST-108` FR4, `component:Button` FR15). The | ||
| * controller's timer scales by the same tempo (pressFeedback.ts). | ||
| */ | ||
| const PRESS_RELEASE_DURATION = `calc(${pressConsts.releaseMs}ms * var(--_astryx-motion-tempo, 1))`; |
There was a problem hiding this comment.
Nothing caps this under reduced motion, and the controller's timer (pressFeedback.ts:190) scales the same way. With prefers-reduced-motion: reduce on a coarse pointer, a tempo: 1.5 theme fades the press over 0.3 s in Chromium, against design:reduced-motion DR4 and DR2's 200 ms cap, and against AST-108 FR4's "tempo may shorten a Reduce Motion cross-fade but never lengthens it". Capping the scaled release at 200 ms under reduced motion, in the stylesheet and the timer, keeps both.
| ): Record<string, string> { | ||
| const knobs = resolveMotionKnobs(config); | ||
| const tokens: Record<string, string> = {}; | ||
| if (knobs.tempoIsSet) { |
There was a problem hiding this comment.
Only a theme that sets tempo writes the --pre-tempo-* copies and --_astryx-motion-tempo. A theme nested inside a tempo: 1.5 theme that sets its own durations inherits the outer values: in Chromium its Spinner turns in 0.73 s instead of its own 0.45 s, and its press release runs at 1.5. Writing the copy beside every --duration-* token a theme writes, generated or explicit, and the tempo property at 1 for a theme without one, stops the leak.
| durations, | ||
| easings: { | ||
| '--ease-standard': standard, | ||
| '--ease-entrance': value('--ease-entrance', standard), |
There was a problem hiding this comment.
For a theme whose only motion setting is an explicit --ease-standard: linear, this reports --ease-entrance and --ease-move as linear, but nothing writes either token, so the CSS keeps cubic-bezier(0.24, 1, 0.4, 1) (architecture:theme-tokens INV6). Given a defined theme, the knobs also come back as tempo 1 and 'standard', because a defined theme keeps them in __axes.motion, not motion.
| content: [ | ||
| { | ||
| type: 'prose', | ||
| text: "A theme sets how motion feels with three knobs in `defineTheme`'s `motion`. They drive the CSS motion tokens and the springs of touch motion from the same values, so both stay in step.", |
There was a problem hiding this comment.
No animator code reads the knobs yet: resolveThemeMotion has no caller, and nothing reads data-astryx-motion-character. In this release the knobs reach no touch spring. The template and the defineTheme JSDoc also say press clocks never follow the knobs, while the press release fade now does.
| }), | ||
| ); | ||
| }, | ||
| TIMER_MS[kind] * (kind === 'fade' ? themeTempo(live.element) : 1), |
There was a problem hiding this comment.
The timer reads the root's data-astryx-motion-tempo, but an adaptation rule that sets tempo changes only --_astryx-motion-tempo inside its media query. For a root tempo: 1.2 with a coarse-pointer rule at 0.8, the stylesheet fades over 160 ms while this clears the state at 240 ms; a rule that raises tempo ends the fade early. Timing the release from the element's resolved --_astryx-motion-tempo, or ending it on animationend, keeps one clock.
|
/approve-spec 5b7ac47 |
|
Posted by Cindy's agent on her behalf. One test before this merges, for
Everything else in this delta matches AST-108. Thanks! |
Rebased onto main as one change from the theme motion knobs branch (spec:AST-108 FR1-FR7): tempo scales every motion duration, character sets the --ease-entrance and --ease-move curves, and reducedMotion marks the theme's root. Spinner's and Skeleton's loops keep their timing under every tempo, the press release fade follows tempo up to the Reduce Motion cap, adaptation rules take tempo and character, an extending theme inherits motion field by field, and theme build checks a defined theme's authored tokens and rules. The motion, token and theme-authoring docs, the theme template and the authoring contract describe the knobs.
…'always' A theme's reducedMotion: 'always' reached only its motion-keyed adaptation rules and the press release fade; components' own Reduce Motion styles, Spinner and Skeleton among them, did not follow it. A theme that set it got a promise most of the page did not keep. The public type now offers only 'system', the default, and the root mark, its custom property, the marked adaptation blocks and the press release's mark arm go with it. The docs, template, changeset and the authoring contract's unshipped note say so.
8e5c8ff to
a2d7f42
Compare
There was a problem hiding this comment.
Changes requested: the motion knobs follow the theme motion spec and existing themes resolve to the same tokens, but two places contradict current spec.
resolveThemeMotionreturnsvar()strings for motion tokens written as references (themeMotion.ts:181-182); resolve them through the token graph asresolveThemeTokensdoes, with a test (architecture:theme-tokensINV6,spec:AST-108FR6).spec:AST-108FR7 asks every listed doc for the knobs, the fixed list and the precedence; the duration and easing token descriptions, the theme-authoring guide's motion row and INV15 still lack the fixed list.
[Automated review]
| const value = (name: string, fallback: string): string => | ||
| theme.tokens?.[name] ?? generated[name] ?? fallback; |
There was a problem hiding this comment.
This reads theme.tokens raw, so a motion token written as a reference comes back unresolved: with tokens: {'--duration-medium': 'var(--duration-slow)'} and tempo: 1.2, this returns var(--duration-slow) while the CSS and resolveThemeTokens(theme, {mode}) give 1170ms. architecture:theme-tokens INV6 asks JavaScript helpers to follow token references, and spec:AST-108 FR6 asks the server and client to resolve the same values. Reading through resolveThemeTokens and keeping expandThemeMotion as the fallback would fix it; a referenced-token case in the FR6 test would pin it.
| title: 'Duration Tokens', | ||
| description: | ||
| 'Motion duration primitives. Three bands: fast (micro-interactions), medium (entrance/exit), slow (continuous). Min/max variants derive from base × ratio.', | ||
| "Motion duration primitives. Three bands: fast (micro-interactions), medium (entrance/exit), slow (continuous). Min/max variants derive from base × ratio. A theme's `motion.tempo` (0.75–1.5) scales every one of them; a duration scale or an explicit token wins over it.", |
There was a problem hiding this comment.
spec:AST-108 FR7's verification row fails "a doc without the knobs, the fixed list, or the precedence". This description names only tempo and no fixed list, so a reader expects every component that reads these tokens, Spinner and Skeleton included, to follow tempo. One sentence naming what never follows the knobs, as the motion topic lists it, would cover it; the easing description at line 271, the guide's motion row and INV15 need the same.
…oc lists what the knobs never move resolveThemeMotion returned a motion token written as a reference as its var() string, where resolveThemeTokens resolves it (architecture:theme-tokens INV6). The var() resolver moves out of tokens.ts into tokenReferences.ts, unchanged, and resolveThemeMotion resolves through the defaults, the knobs' tokens and the theme's own; tokens.ts keeps evaluating color functions on it. The duration and easing token descriptions, the theme-authoring guide's motion row and INV15 now name the fixed list (spec:AST-108 FR4, FR7).
There was a problem hiding this comment.
Changes requested: the motion knobs follow the theme motion spec, existing themes resolve to the same tokens, and resolveThemeMotion now resolves referenced tokens, but three of the docs spec:AST-108 FR7 lists still miss part of what it requires.
- The easing-token description (
scripts/generate-token-docs.mjs:271) names onlycharacterand gives no precedence, and the duration-token description (:261) names onlytempo; both need all three knobs and the precedence sentence the motion topic uses. - The theme template's list of what never follows the knobs (
packages/cli/assets/theme.template.ts:192-194) leaves out the rubber band, the press's cancel distance and gesture thresholds.
Regenerate the token docs with node scripts/generate-token-docs.mjs after editing the descriptions.
[Automated review]
| title: 'Easing Tokens', | ||
| description: 'Easing curves for animations and transitions.', | ||
| description: | ||
| "Easing curves for animations and transitions. `--ease-entrance` and `--ease-move` follow a theme's `motion.character`: `crisp` decelerates without overshoot, `expressive` overshoots slightly, and at `standard` both equal `--ease-standard`. Exits keep their own curve at every character. Neither knob moves finger tracking, momentum, the rubber band, the on-screen keyboard, the press's onset delay, cancel distance and tap flash, hover-intent delays, gesture thresholds, the loops of busy indicators such as Spinner and Skeleton, and the 200 ms Reduce Motion cap.", |
There was a problem hiding this comment.
spec:AST-108 FR7 fails a listed doc "without the knobs, the fixed list, or the precedence". This easing description names only character (while saying "Neither knob") and never says that an explicit --ease-entrance or --ease-move token beats it. Naming tempo, character and reducedMotion in one clause and adding the precedence sentence from the motion topic would cover it; rerun node scripts/generate-token-docs.mjs afterwards.
| title: 'Duration Tokens', | ||
| description: | ||
| 'Motion duration primitives. Three bands: fast (micro-interactions), medium (entrance/exit), slow (continuous). Min/max variants derive from base × ratio.', | ||
| "Motion duration primitives. Three bands: fast (micro-interactions), medium (entrance/exit), slow (continuous). Min/max variants derive from base × ratio. A theme's `motion.tempo` (0.75–1.5) scales every one of them; a duration scale or an explicit token wins over it. Never scaled by it: finger tracking, momentum, the rubber band, the on-screen keyboard, the press's onset delay, cancel distance and tap flash, hover-intent delays, gesture thresholds, the loops of busy indicators such as Spinner and Skeleton, and the 200 ms Reduce Motion cap.", |
There was a problem hiding this comment.
This duration description now carries the fixed list and the precedence, but names only tempo; FR7 asks each listed doc for all three knobs, so one clause naming character and reducedMotion would finish it.
| * Finger tracking, momentum, the keyboard, hover, the press's onset delay | ||
| * and tap flash, and busy loops never follow them; the press release fade | ||
| * follows tempo, up to the 200 ms Reduce Motion cap. |
There was a problem hiding this comment.
FR7 asks the theme template for the fixed list; this one leaves out the rubber band, the press's cancel distance and gesture thresholds, which the motion topic and the authoring guide name.
…and what they never move The duration and easing token descriptions name all three knobs and the precedence, each in its own words, and the theme template's fixed list adds the rubber band, the press's cancel distance and gesture thresholds (spec:AST-108 FR4, FR7). Token docs and the search goldens are regenerated.
There was a problem hiding this comment.
Recommended for approval: the three motion knobs follow the theme motion spec, existing themes resolve to the same public tokens, and the duration-token, easing-token and template docs now carry the knobs, the precedence and the fixed list. Before merge, the repository's spec gate still needs an engineering owner's /approve-spec for this exact head, because the pull request edits the theme authoring contract; the author may record it as an engineering owner. One small fix to the description: check step 2 says a Dialog's entrance overshoots under character: 'expressive', but Dialog still eases on --ease-standard, so for now character reaches only styles that read --ease-entrance or --ease-move.
[Automated review]
|
/approve-spec 71d0814 |
cixzhang
added
the
ci:run
github-actions
Bot
removed
the
needs:spec-owner-review
Keeps main's --ease-exit beside --ease-entrance and --ease-move; the motion knobs never write it, and the easing docs name it as the curve that is the same at every character.
There was a problem hiding this comment.
Changes requested. The motion knobs follow the theme motion spec, and existing themes resolve to the same public tokens, but one docs conflict needs fixing now that #8004 is merged in. The motion topic's Easing section still says there are two easing curves and puts arrivals on --ease-standard, while this pull request's own guidance puts them on --ease-entrance. Please name all four curves and their jobs there, and name --ease-exit beside --ease-entrance and --ease-move in the motion guidance and the theme template, as asked earlier in this thread.
[Automated review]
| { | ||
| type: 'prose', | ||
| text: 'Set transition durations and easing from the motion tokens, so a theme that retunes motion retunes your animations too.', | ||
| text: "Set transition durations and easing from the motion tokens, so a theme that retunes motion retunes your animations too. Ease something arriving on `--ease-entrance` and something moving on screen on `--ease-move`, so it follows the theme's `character`.", |
There was a problem hiding this comment.
This now conflicts with the Easing section at line 53, which (since #8004 merged in) still reads "Two easing curves. --ease-standard decelerates, for things arriving…" above a table that lists four. Please rewrite line 53 to name --ease-standard, --ease-entrance, --ease-move and --ease-exit with their jobs, and name --ease-exit for something leaving here too.
| * yet): | ||
| * tempo — 1 is Astryx's own; 0.75 the quickest, 1.5 the slowest. | ||
| * character — 'crisp' | 'standard' | 'expressive': the entrance and move | ||
| * curves. Exits and fades never overshoot. |
There was a problem hiding this comment.
With #8004 landed, please name --ease-exit here as the curve for something leaving, beside the entrance and move curves, as requested earlier in the thread.
…es, and names --ease-exit - reducedMotion leaves the public motion type until 'always' ships; every theme still resolves to 'system'. - ThemeMotionConfig, ThemeMotionKnobs and MotionCharacter are exported. - The motion guide's Easing section names all four curves and their jobs, and the motion guidance and theme template name --ease-exit for something leaving.
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision: this head rewrites the reducedMotion known gap in the theme authoring contract, so the earlier approval no longer covers the spec text, and an engineering owner, the author included, records the decision by commenting /approve-spec 72881da62404a7fa2e81abfdfe9ef7311f3f0440. That decision covers dropping reducedMotion from the public motion type, which the theme motion spec's FR1, its example and FR7 still state, and the three newly exported knob types. Separately, please reject a partial duration scale written beside a knob: motion: {fast: 100, tempo: 1.2} compiles but drops fast: 100 and renders 210 ms, while FR5 says explicit scale fields win over tempo. The easing-docs request from the last round is fixed.
[Automated review]
| const motion: ThemeMotionConfig | undefined = | ||
| input.motion != null && baseMotion != null | ||
| ? {...baseMotion, ...input.motion} | ||
| : input.motion; |
There was a problem hiding this comment.
A partial scale beside a knob passes the type check and is dropped here without an error: on a theme with no base, motion: {fast: 100, tempo: 1.2} gives --duration-fast 210 ms, {fast: 100, medium: 250, slow: 700, tempo: 1} (no ratio) gives the defaults, and {easing: 'linear', character: 'standard'} never writes --ease-standard, because expandThemeMotion applies a scale only when fast, medium and ratio are all present. The theme motion spec's FR5 says explicit MotionScaleConfig fields win over tempo. Rules already reject this (completeMotion: "must supply fast, medium, and ratio…"); applying the same check to the merged motion here, with a test, would close it.
…not dropped defineTheme refuses a motion that sets a duration scale field without fast, medium and ratio after inheritance, as adaptation rules already do, so explicit scale fields never silently lose to tempo (AST-108 FR5). AST-108's current-state impact names the reducedMotion known gap.
|
/approve-spec 18b09c4 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Recommended for approval: the motion knobs follow the theme motion spec, existing themes keep their tokens, and a partial duration scale written beside a knob now fails with a clear error instead of being silently dropped. An engineering owner approved the spec text this pull request adds for this exact commit. One optional tidy-up: the description still says reducedMotion ships with its 'system' default, but the public type no longer has that field.
[Automated review]
Why
A theme author cannot say how motion should feel, quicker, crisper or reduced, without overriding durations and curves per component.
spec:AST-108(#8613) givesdefineTheme'smotionthree knobs:tempo,characterandreducedMotion. This pull request shipstempoandcharacter, and exports their types (ThemeMotionConfig,ThemeMotionKnobs,MotionCharacter).reducedMotionstays out of the public type until'always'works: component Reduce Motion styles, script readers and the animator must follow it first. Every theme follows each person's setting, as'system'does.What led to it
This comes from the work to make Astryx's touch components behave as native iOS ones do, where a theme's motion must reach springs and press feedback, not only CSS. Themes could scale their duration tokens, but easing was one curve for everything, and Reduce Motion only followed the system setting.
Options and tradeoffs
tempoby name, or read a copy of each duration taken beforetempo.Why this choice
slowkeeps its base's slow band (docs(theme): an extending theme's motion inherits field by field #8748).--ease-exitis feat(tokens): add --ease-exit, the curve BottomSheet already closes on #8004's; this defines none, andcharacternever moves it.reducedMotion: 'always'would have reached only motion-keyed adaptation rules and the press release fade, so a theme setting it would reduce a little of the page and leave Spinner, Skeleton and most components moving. With'system'as the only value the field would do nothing, so the public type leaves it out until every component follows'always'.How to check it
<Theme theme={defineTheme({name: 'Slow', motion: {tempo: 1.5}})}>: it opens 1.5 times slower; a Spinner beside it keeps its speed.character: 'expressive':--ease-entranceand--ease-movetake the overshooting curve, so a style that eases on them overshoots on entrance;--ease-standardand exits do not change. Dialog still eases on--ease-standard, so it does not overshoot yet.--duration-medium: 300mswithtempo: 1.2: the child's is 492 ms.defineTheme({name: 'Partial', motion: {fast: 100, tempo: 1.2}})throws: a duration scale needsfast,mediumandratio, from the theme or its base.Evidence
packages/core/src/theme/themeMotion.test.tsandpackages/core/src/utils/pressFeedback.test.ts. Each new case fails without its part of the change.Writing rubric results
R1 narration, R2 research, R4 rejected alternatives, R5 change references and R6 open questions: none. R3 evidence: INV15's row cites its test file.
Scope