Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
added
the
needs:spec-owner-review
|
AI review status for this pull request.
|
![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 on hold for a spec decision: resolveSettleMs implements spec:AST-108 FR4a, and neither that record nor this head's amendments to spec:AST-094 (FR1, FR4, DEC-15), architecture:theme-application INV1, spec:AST-012 FR4, design:reduced-motion DR2a and architecture:theme-authoring-contract INV15 are on main yet. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or by landing those records on main first. Whatever the decision, please reduce this pull request to its own change (it currently carries 17,988 authored lines from the stacked work beneath it) and address these points:
reducedMotion: 'always'is documented as reducing motion for everyone, but no component style ormatchMediareader keys on its root mark yet, so components keep full motion.resolveSettleMsreturns the full spring settle under Reduce Motion, so a CSS part timed from it the way its example does would fade past the 200 ms Reduce Motion cap.- Merging a child theme's
motionfield by field regenerates an inheritedslowband at the child'sratio, which changes existing extending themes.
[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.
This promises that 'always' reduces motion everywhere. At this head, though, no component style or matchMedia reader keys on [data-astryx-reduced-motion]: see, for example, Spinner's reduced loop, Carousel's scroll behaviour and AppShell's matchMedia check. Please either key those on the mark as well, or say here what 'always' reaches today, which is adaptation rules and the touch motion foundation.
| style: 'unordered', | ||
| 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.", |
There was a problem hiding this comment.
No component reads --ease-entrance or --ease-move yet, so character changes nothing a person sees in today's components. Could this say that components adopt these curves, as the changeset does?
| export function resolveSettleMs( | ||
| element: Element | null | undefined, | ||
| spring: SpringDescription, | ||
| ): number { | ||
| return settlingDuration(toSpring(resolveMotion(element, spring))) * 1000; |
There was a problem hiding this comment.
Under Reduce Motion (the person's setting, or a theme's 'always') the paired spring becomes a cross-fade of 200 ms or less, or an instant change, yet this still returns the full settle: about 0.73 s for springs.settle. The example above sets an inline transitionDuration, which beats a stylesheet's prefers-reduced-motion rule, so a scrim following it would fade past design:reduced-motion DR2's 200 ms cap. Could this return reducedMotionFade(element)'s length when motion is reduced?
| ): Record<string, string> { | ||
| const knobs = resolveMotionKnobs(config); | ||
| const tokens: Record<string, string> = {}; | ||
| if (knobs.tempoIsSet) { |
There was a problem hiding this comment.
The pre-tempo values are written only when this theme sets tempo, and custom properties inherit. So a nested theme with its own duration scale or explicit durations, inside a theme that sets tempo, gives its Spinner and Skeleton the ancestor's pre-tempo value instead of its own duration. Writing the pre-tempo values whenever a theme writes duration tokens, or resetting them otherwise, would keep the nearest theme in charge.
| const baseMotion = base?.__axes?.motion; | ||
| const motion: ThemeMotionConfig | undefined = | ||
| input.motion != null && baseMotion != null | ||
| ? {...baseMotion, ...input.motion} | ||
| : input.motion; |
There was a problem hiding this comment.
Merging the legacy scale fields changes existing extending themes. A child that sets fast, medium and a different ratio over a base that sets slow now regenerates the slow band at the child's ratio. For example, with a base of slow: 800, ratio: 0.75 and a child of ratio: 0.5, --duration-slow-min goes from 600 ms to 400 ms; before, the child kept its base's slow tokens. Merging only the knobs would leave released themes unchanged.
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
meta-cla
Bot
added
the
CLA Signed
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the new theme-motion spec (spec:AST-108) and on updating the existing spec:AST-094 FR4 and DEC-11, which keep motion roles out of themes, plus this pull request's amendments to the theme, reduced-motion, Spinner and Skeleton records; any engineering owner, the author included since they are one, records it by commenting /approve-spec <full head SHA> on this pull request. Separately from that decision, these need changes:
- Rebase onto the landed stack so this pull request carries only
resolveSettleMsand the glide change. - Make
reducedMotion: 'always'reach component styles, makeresolveSettleMsfollow Reduce Motion, keep a nested theme's duration scale timing Spinner and Skeleton, and keep extending themes' durations unchanged or name the change in the changeset.
[Automated review]
| * follows a duration scale or an explicit token but never `tempo` (FR4). | ||
| */ | ||
| export function preTempoName(token: string): string { | ||
| return token.replace(/^--duration-/, '--pre-tempo-duration-'); |
There was a problem hiding this comment.
These --pre-tempo-duration-* names are projected here and read by Spinner and Skeleton, but tokens.stylex.ts declares no such token and neither the token docs nor the theme template list them. architecture:theme-tokens INV1 on main says "A core token name and its default originate in tokens.stylex.ts; projections do not introduce names." Declare them there, or amend that record with AST-108.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the new architecture:theme-authoring-contract INV15 clause that an explicit motion token anywhere in an extending theme's lineage beats the tokens the child generates: current spec does not state it, and it changes released output. Any engineering owner, the author included if they are one, can record that decision by commenting /approve-spec <full head SHA> on this pull request, or by landing the clause on main. Whatever the decision, these points need changes:
- Land the stacked pull requests and rebase, so the diff carries only
resolveSettleMsand its tests (it carries 17,919 authored lines today). - Make
reducedMotion: 'always'reach component styles and script readers asspec:AST-108FR6 requires, and makeresolveSettleMsfollow Reduce Motion (FR4) and end with the spring the animator plays (FR4a). - Keep a nested theme's CSS durations, springs and Spinner and Skeleton loops on one theme, declare
--pre-tempo-duration-*intokens.stylex.ts, and name the extending-theme output changes in the changeset.
[Automated review]
| element: Element | null | undefined, | ||
| spring: SpringDescription, | ||
| ): number { | ||
| return settlingDuration(toSpring(resolveMotion(element, spring))) * 1000; |
There was a problem hiding this comment.
Under Reduce Motion (the person's setting or a theme's 'always'), the animator plays the paired spring as a cross-fade of 200 ms or less on a coarse pointer, or at once on a fine one. This line still returns the full settle: about 735 ms for springs.settle, 1,102 ms at tempo: 1.5. The example above sets an inline transitionDuration, so a scrim timed from it would fade past design:reduced-motion DR2's 200 ms cap. Could this return reducedMotionFade(element)'s duration when that is non-null?
Separately, the animator ends a translate when it is within 0.05 px (SETTLE_THRESHOLD), so a 600 px sheet on springs.settle ends at about 951 ms, while this gives 735 ms. That puts the sheet and its scrim 9–15 frames apart, against FR4a's one-frame check.
| export function themeMotionKnobs( | ||
| element: Element | null | undefined, | ||
| ): ThemeMotionKnobsAt { | ||
| const scope = element?.closest?.(THEME_SCOPE); |
There was a problem hiding this comment.
This reads only the nearest theme scope, so a nested theme that sets no tempo plays its springs at tempo 1. Its CSS --duration-* tokens, though, are the outer theme's scaled values: a nested theme writes only its own tokens, and custom properties inherit. The press release timer also walks up to any ancestor's tempo. In that subtree CSS motion and springs run on different tempos (spec:AST-108 FR3). Should each knob resolve from the same scope its CSS tokens come from?
| ): Record<string, string> { | ||
| const knobs = resolveMotionKnobs(config); | ||
| const tokens: Record<string, string> = {}; | ||
| if (knobs.tempoIsSet) { |
There was a problem hiding this comment.
The --pre-tempo-duration-* values are written only when this theme sets tempo. A nested theme with its own duration scale inside a tempo theme therefore leaves Spinner and Skeleton on the outer theme's pre-tempo values, and its scale stops timing them (FR4: "so a MotionScaleConfig still times them"). Writing them whenever a theme writes duration tokens would fix that. These names are also not declared in tokens.stylex.ts, which architecture:theme-tokens INV1 requires of every core token name.
| input.motion != null && baseMotion != null | ||
| ? {...baseMotion, ...input.motion} | ||
| : input.motion; | ||
| const lineageMotionTokens = |
There was a problem hiding this comment.
Re-applying the base's own motion tokens as the child's explicit tokens changes released output. A child whose duration scale sets medium: 300, over a base with tokens: {'--duration-medium': '500ms'}, renders 500 ms here and 300 ms on main. Neither the precedence order on main nor spec:AST-108 FR5 states this rule; it is the INV15 clause awaiting an engineering owner's decision.
| are the defaults. A duration scale's durations win over `tempo`, and | ||
| explicit tokens win over both. An extending theme inherits motion field by | ||
| field: a duration-scale field or knob the child leaves unset keeps its | ||
| base's value, and an explicit token anywhere in the lineage still wins. Each duration's |
There was a problem hiding this comment.
"an explicit token anywhere in the lineage still wins" is new: this record's precedence list still puts the resolved base below generated values, and spec:AST-108 FR5 says only that explicit tokens win over tempo and character. An engineering owner needs to approve or reject this clause, for example with /approve-spec <full head SHA>.
| 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 only the animator, the layout glide and when: {motion} adaptation rules key on the reduced-motion mark. Component styles (Spinner's reduced loop, Skeleton's pulse) and script readers (AppShell, Carousel, MobileNav) still read prefers-reduced-motion alone, so 'always' does not yet reduce motion for everyone, as spec:AST-108 FR6 requires.
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [feat] A theme's `motion` takes three knobs beside the duration scale: `tempo` (0.75–1.5) scales every motion duration, `character` (`'crisp'`, `'standard'` or `'expressive'`) sets the new `--ease-entrance` and `--ease-move` tokens, and `reducedMotion: 'always'` marks the theme's root so motion is reduced for everyone. A theme without them renders exactly as before; a duration scale and explicit tokens win over the knobs, and Spinner's and Skeleton's loops keep their timing under every tempo. The motion, token and theme-authoring docs and the theme template describe the knobs. |
There was a problem hiding this comment.
"A theme without them renders exactly as before" misses two extending cases. First, a child scale without slow, over a base that sets it, now regenerates the slow band at the child's ratio (base slow: 800, ratio: 0.75 with child ratio: 0.5: --duration-slow-min 600 ms to 400 ms). Second, a base's explicit motion token now beats the child's own scale. Could the changeset name both?
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on this head's change to architecture:theme-authoring-contract: it replaces the accepted statement that resolveThemeMotion returns role tables for the animator and for server code with one where the animator reads the theme's custom properties and the resolver serves server code only. Any engineering owner, the author included since they are one, can record that decision by commenting /approve-spec <full head SHA> on this pull request, or by landing the amended record on main. Whatever the decision, these need changes:
- Land the stacked pull requests and rebase, so the diff carries only
resolveSettleMsand its tests (it carries 16,980 authored lines today). - Make
reducedMotion: 'always'reach the remaining component styles and script readers, asspec:AST-108FR6 requires, or hold the option back until it does. - Make
resolveSettleMsfollow Reduce Motion and end with the spring the animator plays (FR4, FR4a), and make the FR4a glide test set--_astryx-motion-tempoand--_astryx-motion-characterinstead of attributes nothing reads.
[Automated review]
| it('lasts what a CSS part timed from springs.settle lasts in the same theme, so the two land together (spec:AST-108 FR4a)', async () => { | ||
| await changeOnce( | ||
| '', | ||
| 'data-astryx-theme="lively" data-astryx-motion-tempo="1.2" data-astryx-motion-character="expressive"', |
There was a problem hiding this comment.
Nothing at this head reads data-astryx-motion-tempo or data-astryx-motion-character: themeMotionKnobs reads the --_astryx-motion-tempo and --_astryx-motion-character custom properties. So this case runs at Astryx's own knobs and would still pass if the glide ignored the theme. Passing '--_astryx-motion-tempo: 1.2; --_astryx-motion-character: expressive' as the style argument, as the tempo case above does, would make it test what its name says.
| // The loop reads the duration before a theme's tempo scales it | ||
| // (spec:AST-108 FR4). | ||
| default: `var(--_astryx-pre-tempo-duration-slow-min, ${durationVars['--duration-slow-min']})`, | ||
| '@media (prefers-reduced-motion: reduce)': '3s', |
There was a problem hiding this comment.
Under a theme's reducedMotion: 'always' with the person's own setting off, this rule never applies, so the loop keeps its full speed; spec:AST-108 FR6 asks every style keyed on prefers-reduced-motion to key on the theme's mark too. Skeleton's pulse and the other component styles and script readers the record now lists as a known gap behave the same way.
|
|
||
| - An internal, pure `resolveThemeMotion(theme)` returns a theme's motion | ||
| knobs, durations, and easings for server code; it touches no DOM and has no | ||
| import-time side effects (`spec:AST-108` FR6). The animator reads the knobs |
There was a problem hiding this comment.
This replaces the record's accepted statement that resolveThemeMotion returns role tables "for the animator and for server code" with a new reading: the animator reads the theme's custom properties and the resolver serves server code. An engineering owner needs to approve that, for example by commenting /approve-spec <full head SHA>.
A theme's `tempo` reached the CSS motion tokens but not the touch springs, so a slowed theme played its fades and its springs at two speeds. resolveMotion, the one door every touch spring and role duration passes through, now reads the tempo of the theme the element is in: the private property the theme writes beside its duration tokens. A spring keeps its damping ratio and takes tempo times as long, and a duration role scales. The layout glide resolves springs.settle through it at the area. Finger tracking, ProgressBar's busy loop and curves stay as measured (spec:AST-108 FR4). A theme with only a duration scale writes no tempo, so its springs stay at tempo 1 (FR5). The springs don't follow `character` yet; that part waits on its record. A `Core/Theme motion tempo` story shows three themes side by side for a feel check on a touch screen, and its glide is the browser proof's fixture.
A CSS part that plays alongside a spring, such as a scrim fading under a sheet, has to start and land with it. Timed on its own clock, it drifts once a theme's tempo changes the spring. resolveSettleMs(element, spring) returns the resolved spring's settling duration at the element's tempo. That is the length the layout glide plays for the same role, so a glide and a part timed from it land together. A fixed role, or a spring that is not a role, gives its own. No component uses it yet.
491f6ac to
a995a80
Compare
vjeux
changed the title
github-actions
Bot
removed
needs:design-review
Stack: #8677 → #8686
Stacked on #8677, the theme's
tempofor the springs. Review only this pull request's commit: vjeux@a995a80User need
A CSS part that plays alongside a spring has to start and land with it. Two examples are a scrim fading under a sheet, and a menu's content under its grow. Timed on its own clock, it drifts as soon as a theme's
tempochanges the spring.Problem and solution fit
resolveSettleMs(element, spring)returns how long the spring takes to settle at the tempo of the themeelementis in, in milliseconds. A CSS part takes that number as its duration. It is the resolved spring's settling duration, the same length the layout glide plays for the role. So a glide and a part timed from it land together.Current specification
spec:AST-108FR4a (current) says coordinated animations share one clock: "the CSS part is timed from the spring's computed settle time". Its examples include BottomSheet's scrim, "whose fade lasts the sheet's computed settle time". FR3 has the tempo scale the spring.characterwaits on #9704, as in #8677.Public delta and caller burden
Design and alternatives
Evidence
resolveMotion.tempo.test.ts:springs.settle.animatedLayout.test.ts: under tempo 1.2, the glide's duration equalsresolveSettleMs(area, springs.settle).resolveSettleMsdoesn't exist there.Scope
Testing
Gate at
a995a809b6:pnpm check:repoand changeset coverage pass.pnpm test:ui: 19,960 passed.pnpm test:node: 16,430 passed. 4 CLI cases timed out while the box was loaded (doctor icon-sets, debug-result-summary ×2, resolve-theme). The 3 files pass on this head and on main side by side at the same moment, 45/45 each.