Skip to content

feat(core): createAnimator and useAnimator animate one element - #8653

Draft
vjeux wants to merge 1 commit into
meta:mainfrom
vjeux:uikit/f1-public-animator
Draft

vjeux wants to merge 1 commit into
meta:mainfrom
vjeux:uikit/f1-public-animator

Conversation

@vjeux

@vjeux vjeux commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

User need

A product that drags, flicks or throws an element has no public way to give it the motion Astryx's own components use: today it copies springs and decay curves by hand and loses Reduce Motion handling.

Problem and solution fit

The spec picks the small handle, so a product's drag stays in step with Core's components. Following the maintainers' API decision, useAnimator(ref) takes the element's ref, as the gesture hooks do, so a drag and an animation can share one element. The factory, the hook, both docs pages, both examples and their tests ship together: FR5 asks each hook to ship with its docs and a playing-cards example, and FR6 builds the hook on the factory.

Current specification

spec:AST-107 FR3 opens the touch components' animator as a public handle, with a non-React factory under the hook (FR6).

Public delta and caller burden

  • createAnimator(element), from the package root only, returns animate, decay, set, get, getVelocity, stop and destroy over x, y, scale, rotate and opacity.
  • useAnimator(ref), from @astryxdesign/core/hooks, takes the element's ref and returns the animator. After every commit it follows the element behind the ref: it attaches a node that mounts, moves to one that replaces it, and lets an unmounted one go; a remount, or a node that replaces it, starts at rest. It does this through useAttachment, the attach lifecycle the gesture hooks share, so it has no lifecycle of its own.
  • decay coasts any channel; opacity stops at 0 or 1. Under Reduce Motion a thrown x or y keeps its momentum; anything else fades on a touch screen or lands at once with a mouse.
  • The internal factory is renamed createMotionAnimator, so no internal factory shares a public factory's name. Its callers on main are renamed with it: the Slider's touch controller (feat(Slider): every pointer drags a thumb by its travel, as UISlider does #8190), the Switch's touch controller and its test's stand-in (feat(Switch): the touch presentation, for every pointer #9636), and the pager's tests (chore(core): a pager that pages like a UIKit paging scroll view #8146).
  • In both playing-cards examples, the card is an image (role="img") named by its rank and suit, renamed on each deal, and one card leaves at a time: a Discard press while a card leaves, by a flick or by the button, deals no second card, and a press on the leaving card does nothing. A press on a card that is fading in moves only its position, so the fade runs on. A drag that the browser cancels springs back instead of being thrown.
  • When the element behind useAnimator's ref unmounts, or a node replaces it, the animator's values return to rest, so a remount or a new node starts from the element's resting values.
  • spec:AST-107 records the owner's decision on four edge behaviors of the public animator and pan gesture as DEC-4. FR1: a cancelled pan reports velocity {0, 0}. FR6: createPanGesture's destroy() during a drag reports cancelled once, usePanGesture's unmount reports nothing, and an opacity decay stops at 0 or 1, which createAnimator already does. FR3: a remount starts from the element's resting values. The pan sentences describe the public pan that comes next in this stack, which already behaves this way; they sit here so the record changes in one place.
  • The core API report gains the animator's rows: function createAnimator, hook useAnimator, and the Animator, AnimatorAnimateOptions, AnimatorChannel, AnimatorDecayVelocity, AnimatorHandle, AnimatorSnapshot and AnimatorValues types. spec:AST-107 FR3 decides the handle and its channels, and FR6 the factory under the hook. Regenerating the report on this head also rewrites rows that main changed without updating it (text input props, some style hashes); those are left out, so this pull request adds only its own rows.

Left out.

Effect on other open pull requests. The internal factory's rename to createMotionAnimator breaks six open drafts that import it as createAnimator from the motion module: #8065, #8145, #8189, #8496, #8677 and #8686. Each one renames that import when it rebases; the factory's signature is unchanged. A temporary internal alias is not possible, since no internal factory may share the public factory's name.

Size. import {createAnimator} from '@astryxdesign/core' adds 37.9 KB minified, 14.0 KB gzip, and import {useAnimator} from '@astryxdesign/core/hooks' adds 38.4 KB minified, 14.2 KB gzip, measured with the size report's build options. Neither import reaches any gesture module, which the touch-support bundle test checks.

Pull request size. This pull request's own commit is 2,542 lines with tests, and no part of it can stack apart. spec:AST-107 FR5 reads "Each hook ships with docs and a playing-cards example", so the two playing-cards templates, their test and their registrations (709 lines) ship with the hooks. The rest is the two exports with their docs, changeset and API report rows (716 lines), and the tests that cover them (1,117 lines), which cannot land apart from the code they test.

Design and alternatives

  • Expose the internal animator: everything, but every spring, curve and option becomes a compatibility promise.
  • A small handle on five channels with no spring choice: products match Core, at the cost of tuning.

Evidence

  • Red-first:
    • Discard twice, and Discard during a flick's fade: with the one-at-a-time guard removed from both examples, all four cases fail (two cards are dealt); with it, they pass. The recorder models takeover as the animator does: a new opacity target ends a running fade.
    • A cancelled drag after fast moves: fails on the previous examples, which threw the card, and passes here.
    • An unmount: "an unmount lets the element go and drops the values; a remount starts at rest" fails on the previous factory, which kept them, and passes here.
    • A replacing node: "follows the element behind the ref when the node is replaced, and the new node starts at rest" fails on the previous factory, which handed the values to the new node, and passes here.
    • A press on a card fading in: "a press on the card being dealt moves only its position: its fade-in runs on". With the press stopping every channel, the card stayed part-faded and shrunk after a gentle release; now only the position stops. It fails on a version with only the leaving-card guard.
    • A press on a leaving card: "a press on a card that is leaving is ignored: its fade runs on, and one card is dealt after it". On the previous examples the press stopped the fade, so the next card was dealt under the finger and the first move pulled it off its place. The recorder's stop now ends running fades, as the animator's does.
    • The card's role: the cases find the card by role="img" and its name, which fails on the previous examples.
    • The dealt card's name: "names the dealt card by its rank and suit" finds the next card by its name after a discard. It fails on the previous plain-JavaScript example, whose card stayed "Top card".
    • useAnimator's module test checks that the hook imports only the factory, useAttachment and React, and calls useAttachment(ref, attach, null).
  • Gates. On this head, on main 6690e33: pnpm install --frozen-lockfile, build, type check, test:ui (18,899), the node suites (16,051, with 2 expected failures), check:repo, ASTRYX_STRICT_LINT=1 pnpm lint (0 errors), check:changesets, changeset coverage and check:knowledge pass. On a fresh Storybook build, the accessibility contracts of the two components whose touch controllers are renamed pass: Switch's (12 of 12) and Slider's, with its geometry contract (30 of 30). The public and internal animators' browser specs pass ×3 in Chromium pinned to 2 cores at 1 worker (72 of 72); the Switch interaction specs pass there too (58 of 58); and under the CI WebKit job's command both sets pass (67 passed; 15 skip by their own engine conditions). Merged with current main 7f1bace, the type check, the CLI's template, golden and doc-input tests (1,054) and the animator, Switch and template tests (168) pass.

Writing rubric results

spec:AST-107, with the sentences added to FR1, FR3 and FR6 and the new DEC-4:

R1 narration: none; the added sentences and DEC-4 state what the system does, in the present tense. DEC-4's decider and date sit in its Decider line, as in DEC-1 to DEC-3.
R2 research: none in the record.
R3 evidence: no verification cell names a pull request, issue or commit.
R4 rejected alternatives: DEC-4 rejects four, in one Rejected: line inside the decision; the other three decisions keep theirs.
R5 change references: none.
R6 open questions: the section reads "None", and nothing the added text answers was open there.

Scope

  • One capability is added.
  • Unrelated fixes, cleanup, and policy changes were removed or split.
  • Consumer docs, focused tests, and migration evidence ship with the feature.
  • Public text and artifacts contain no internal Meta context.

Testing

  1. In the useAnimator playing-cards example, fling the top card: it coasts away, and the next card is dealt once it has faded.
  2. Drag the card a little and let go: it springs back on its release velocity.
  3. With Reduce Motion on, a fling still follows your throw, and the next card fades in.
pnpm exec vitest run --project ui packages/core/src/animator packages/core/src/hooks/useAnimator.test.tsx
pnpm test:interaction packages/core/src/animator/__tests__/createAnimator.interaction.browser.spec.ts --project chromium

@vercel

vercel Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
astryx Ready Ready Preview Nov 3, 2026 4:33am UTC

Request Review

@astracat-bot

astracat-bot Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

AI review status for this pull request.

Review status Updated
🔄 In progress (for maintainers only) Nov 3, 2026, 4:33 AM UTC

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval waits on an engineering owner's decision about the governing spec: spec:AST-107 FR3–FR6, which defines createAnimator, the @astryxdesign/core/animator entry and useAnimator, is only in open #8600. On main, architecture:react-component-runtime INV13 and spec:AST-017 FR21 still keep motion internal, and architecture:public-component-api INV21 has no carve-out yet for a hook that wraps a factory. Any engineering owner can settle it by landing #8600 or by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision, these need fixing:

  • Land #8076 first and rebase: this diff carries 17,491 authored lines, and that stack alone differs from main by 15,672.
  • scripts/motion-node-load.test.mjs still imports the renamed createAnimator from motion/animator and fails.
  • The two playing-card templates need a Changeset naming @astryxdesign/cli, a refreshed ShadCN route lock and the Sandbox's static route list; lint, docsite-data and build-sandbox fail without them.
  • The templates should read zero release velocity after a still pause and deal the next card once the old one fades, and the docs should call animate's motion Astryx's state-change spring rather than the theme's, since roles stay internal under spec:AST-094 DEC-11.

[Automated review]

Comment thread scripts/motion-node-load.test.mjs Outdated
* the press paint's styles).
*/
const ENTRY = `
export {createAnimator, animatorFor, SETTLE_THRESHOLD} from './motion/animator';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still re-exports createAnimator from ./motion/animator, which this PR renames to createMotionAnimator. esbuild leaves the binding undefined, and the child-process test fails with TypeError: m.createAnimator is not a function. Lines 103, 106 and 115 need the same rename.

/** The release velocity in px/s, over the drag's last 80 ms. */
function releaseVelocity(samples: Sample[]): {x: number; y: number} {
const last = samples.at(-1);
const first = samples.find(s => last != null && last.t - s.t <= 80);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The release velocity comes from the last 80 ms of moves and ignores how long the pointer rested before pointerup. A drag that stops and holds still for 600 ms before release still throws the card away (Chromium), although the template says a gentle release springs it back. spec:AST-100 FR2 reads zero after a still pause. Passing the release's timeStamp and returning zero when the newest sample is older than about 80 ms would fix it, here and in the plain-JavaScript template's onUp.

const speed = Math.hypot(velocity.x, velocity.y);
const boost = speed < MIN_THROW ? MIN_THROW / Math.max(speed, 1) : 1;
const throwX = speed < 1 ? MIN_THROW : velocity.x * boost;
await Promise.all([

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This waits for the whole coast, about 800 px at 1,600 px/s. After Discard, the table stays empty and the live region silent for 4.55 s before the next card is dealt. Dealing and announcing once animate({opacity: 0}) resolves, then calling stop(), keeps the keyboard path responsive.

name: 'animate',
type: '(target: {x?, y?, scale?, rotate?, opacity?}, options?: {velocity?}) => Promise<void>',
description:
"Springs the given channels to `target` from wherever they are on screen, on the theme's state-change motion, and resolves when they arrive (or as soon as a later call takes them over). `x` and `y` are px, `scale` is 1 at the element's size, `rotate` is degrees and `opacity` runs from 0 to 1. Pass `velocity` (units per second, such as a drag's release velocity) to start with it; without it a channel keeps the velocity it has on screen, so interrupting a motion never jumps.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"On the theme's state-change motion": the animator plays springs.stateChange's table value, and no theme can retune it while spec:AST-094 DEC-11 keeps the roles internal (the motion README says the same). "Astryx's state-change spring" would be accurate; the same wording is at line 77, in useAnimator.doc.mjs lines 33 and 38, in the changeset and in the createAnimator JSDoc.

@@ -0,0 +1,7 @@
---
'@astryxdesign/core': patch

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This names only @astryxdesign/core, but the two new playing-card templates ship as @astryxdesign/cli output, and CI lint's changeset coverage fails on that. A separate [feat] changeset naming @astryxdesign/cli would clear it (spec:AST-017 FR7).

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval waits on an engineering owner's decision about the governing spec: spec:AST-107 FR3–FR6, which defines createAnimator, the @astryxdesign/core/animator entry and useAnimator, is only in open #8600, while on main, architecture:react-component-runtime INV13 and spec:AST-017 FR21 keep the animator internal and architecture:public-component-api INV21 has no exception yet for a hook over a factory. Any engineering owner can settle it by landing #8600 or by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision, these need fixing:

  • Land #8076 first and rebase: this diff carries 17,530 authored lines, 15,672 of them from that stack.
  • scripts/motion-node-load.test.mjs still imports the renamed createAnimator from motion/animator (lines 31, 103, 106 and 115), so its child-process test fails.
  • useAnimator.doc.mjs line 33 still says calls work before the ref attaches, which this head no longer does.
  • The templates should read zero release velocity after a still pause and deal the next card once the old one fades, and the docs and JSDoc should drop usePanGesture and useAnimatedLayout, which the package does not have yet, and call the motion Astryx's state-change spring rather than the theme's.

[Automated review]

Comment thread scripts/motion-node-load.test.mjs Outdated
* the press paint's styles).
*/
const ENTRY = `
export {createAnimator, animatorFor, SETTLE_THRESHOLD} from './motion/animator';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

./motion/animator now exports createMotionAnimator, not createAnimator, so this re-export finds nothing and m.createAnimator(null) at line 103 throws in the child process: the second test fails whether the bundler stops on the missing name or leaves it undefined. Lines 103, 106 and 115 need the same rename.

name: 'animator',
type: 'AnimatorHandle',
description:
"The element's animator, the same object on every render: `animate(target, {velocity?})` springs channels to a target on the theme's state-change motion, `decay({x?, y?})` coasts them from a release velocity, `set(values)` jumps them, `get()` and `getVelocity()` read them, and `stop()` halts them. The channels are `x` and `y` (px), `scale` (1 at the element's size), `rotate` (degrees) and `opacity` (0 to 1). `animate` and `decay` return a promise that resolves when the motion ends. Calling it never re-renders the component, and it works before the ref attaches: the element shows its values when it mounts.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"It works before the ref attaches: the element shows its values when it mounts" no longer holds: before the ref attaches every call does nothing, as line 27 and the hook's test an animator used before its ref attaches does nothing and resolves at once say.

Comment on lines +33 to +41
function releaseVelocity(samples: Sample[]): {x: number; y: number} {
const last = samples.at(-1);
const first = samples.find(s => last != null && last.t - s.t <= 80);
if (last == null || first == null || last.t === first.t) {
return {x: 0, y: 0};
}
const seconds = (last.t - first.t) / 1000;
return {x: (last.x - first.x) / seconds, y: (last.y - first.y) / seconds};
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 80 ms window is measured back from the newest move, not from the release. After a fast drag that stops and holds still, no more moves arrive, so the release still reads the earlier speed and throws the card, although the copy says a gentle release springs back. Passing the release event's timeStamp and returning zero when the newest sample is older than the window would fix it, here and in the plain-JavaScript template's onUp.

Comment on lines +55 to +58
await Promise.all([
animator.decay({x: throwX, y: velocity.y * boost}),
animator.animate({opacity: 0}),
]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This waits for the whole coast: from the Discard button, 1,600 px/s travels about 800 px and settles about 4.5 s later, so the table stays empty and the live region silent long after the card has faded. Dealing and announcing once animate({opacity: 0}) resolves, then calling stop(), keeps the keyboard path responsive.

export interface AnimatorAnimateOptions {
/**
* Each channel's velocity at the start, such as a drag's release velocity
* (`usePanGesture` reports it at `ended`). A channel without one keeps the

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usePanGesture is not in the package yet, so this public JSDoc points callers to a hook they cannot import.

],
usage: {
description:
"Animates one element with the motion Astryx's own components use, without React. `animate` springs an element to new values on the theme's state-change motion, `decay` flings it with the momentum of a released scroll view, and `set` moves it directly, so a card can follow a finger, snap back and be thrown away. Use the `useAnimator` hook in React. To animate an area's layout as its items change, use `useAnimatedLayout`; for what a component already animates, such as a sheet's detents or a carousel's snapping, use the component's own props.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two claims here go beyond the package: no theme can retune springs.stateChange while motion roles stay internal (spec:AST-094 DEC-11), so "Astryx's state-change spring" would be accurate, and useAnimatedLayout does not exist in the package yet. The same wording is at line 38, in useAnimator.doc.mjs lines 33 and 38, in the changeset and in the createAnimator JSDoc.

vjeux added a commit to vjeux/astryx that referenced this pull request Oct 27, 2026
…duction registry checks) into snap points
vjeux added a commit to vjeux/astryx that referenced this pull request Oct 27, 2026
…oduction registry checks

The same two checks meta#8653's fix meets, for this pull request's own docs:
- createDragToSnapPoints' doc is a core doc that component <Name> reads
  but the component list leaves out (a vanilla factory), so the CLI's doc
  inputs test pins it.
- The useDragToSnapPoints example imports the hook from the released
  hooks subpath, so the hidden production registry built it against the
  published core, which has no such hook yet. Its doc now names
  useDragToSnapPoints among the components it uses, so production skips
  it until the hook is released. A latest-target generate shows the item
  without this line and drops it with it.

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval waits on an engineering owner's decision about the governing spec: spec:AST-107 FR3–FR6, which defines createAnimator, the @astryxdesign/core/animator entry and useAnimator, is only in open #8600, while on main, architecture:react-component-runtime INV13 and spec:AST-017 FR21 keep the animator internal and architecture:public-component-api INV21 has no exception yet for a hook over a factory. Any engineering owner can settle it by landing #8600 or by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision, these need fixing:

  • Land #8076 first and rebase: this diff carries 17,538 authored lines, 15,669 of them from that stack.
  • useAnimator.doc.mjs line 33 still says calls work before the ref attaches, which this head no longer does.
  • Both templates should read zero release velocity after a still pause, and deal and announce the next card once the old one fades rather than after the whole 4.5 s coast.
  • The docs and JSDoc should drop usePanGesture and useAnimatedLayout, which the package does not have yet, and call the motion Astryx's state-change spring rather than the theme's.

[Automated review]

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval waits on an engineering owner's decision about the governing spec: spec:AST-107 FR3–FR6, which defines createAnimator, the @astryxdesign/core/animator entry and useAnimator, is only in open #8600, while on main, architecture:react-component-runtime INV13 and spec:AST-017 FR21 keep the animator internal and architecture:public-component-api INV21 has no exception yet for a hook over a factory. Any engineering owner can settle it by landing #8600 or by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision, these need fixing:

  • Land #8076 first and rebase: this diff carries 17,663 authored lines, 15,794 of them from that stack.
  • useAnimator.doc.mjs line 33 still says calls work before the ref attaches, which the hook does not do.
  • Both templates should read zero release velocity after a still pause, and deal and announce the next card once the old one fades rather than after the whole 4.5 s coast.
  • The docs and JSDoc should drop usePanGesture and useAnimatedLayout, which the package does not have yet, and call the motion Astryx's state-change spring rather than the theme's.

[Automated review]

@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

PR Analysis Report

Current run: The current CI run concluded skipped. Current analysis is unavailable.


Generated by PR Enrichment workflow | View current CI run

@github-actions github-actions Bot added the needs:spec-owner-review Current knowledge records await owner approval label Oct 27, 2026

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes: the public createAnimator and useAnimator match spec:AST-107 FR3–FR6, which is now on main, but this branch also carries the open #8076 and #8677 stacks (19,819 authored lines in all), so please land those first and rebase onto main. These fixes are needed in this pull request's own files:

  • useAnimator.doc.mjs line 33 says calls work before the ref attaches; they do nothing until it does.
  • Both playing-card templates should read zero release velocity after a still pause (a 150 px drag held still for 600 ms is still thrown away).
  • Both templates should deal and announce the next card once the discarded one fades, not after the whole 4.5 s coast.
  • The docs and the velocity JSDoc should drop useAnimatedLayout and usePanGesture, which the package does not export yet.

[Automated review]

name: 'animator',
type: 'AnimatorHandle',
description:
"The element's animator, the same object on every render: `animate(target, {velocity?})` springs channels to a target on the theme's state-change motion, at its tempo and character, `decay(velocity)` coasts them from a release velocity, `set(values)` jumps them, `get()` and `getVelocity()` read them, and `stop()` halts them. The channels are `x` and `y` (px), `scale` (1 at the element's size), `rotate` (degrees) and `opacity` (0 to 1). `animate` and `decay` return a promise that resolves when the motion ends. Calling it never re-renders the component, and it works before the ref attaches: the element shows its values when it mounts.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"It works before the ref attaches: the element shows its values when it mounts" no longer holds: before the ref attaches every call does nothing and resolves at once, as line 27, createAnimator.ts (inactive()) and the hook's test say.

}
origin = null;
const last = samples.at(-1);
const first = samples.find(s => last != null && last.t - s.t <= 80);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The window is measured back from the newest move, not from the release, so a drag that stops and rests before pointerup is still thrown: a 150 px drag held still for 600 ms lands at about 828 px in Chromium. Passing the release event's timeStamp to onUp and returning zero when the newest sample is older than 80 ms would fix it.

async function discard(vx: number, vy: number) {
const speed = Math.hypot(vx, vy);
const boost = speed < MIN_THROW ? MIN_THROW / Math.max(speed, 1) : 1;
await Promise.all([

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This waits for the whole coast, so the next card comes about 4.5 s after Discard although the card is transparent long before. Dealing once animator.animate({opacity: 0}) resolves, then calling stop(), keeps the keyboard path responsive.

@github-actions github-actions Bot removed the needs:spec-owner-review Current knowledge records await owner approval label Nov 2, 2026

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes: createAnimator and useAnimator match spec:AST-107 FR3–FR6, and the spec:AST-094 FR43 sentence this branch carries has an engineering owner's approval for this head, but two things need to change before it can merge.

  • Land #8676 (or drop it from this branch) and merge main, so the diff holds only the animator: 3,026 of its 5,549 authored lines are #8676's.
  • In the plain-JavaScript playing-cards example, name the card by its rank and suit and update that name on each deal; today every card is announced as "Top card".

[Automated review]

const card = document.createElement('div');
card.textContent = DECK[0];
card.setAttribute('role', 'img');
card.setAttribute('aria-label', 'Top card');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With role="img" the rank and suit text is presentational, so this name is all a screen reader announces, and it stays "Top card" after every deal (dealNext changes only textContent). Naming the card by rank and suit here and updating the label in dealNext, as the React example does with card.name, lets screen-reader users hear which card is on top. The template test that finds the card by 'Top card' would change with it.

*/
private fingerTargets: ReadonlyArray<Node> = [];
private nextId = 1;
/** The wheel gesture in progress. */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: this comment and the one on wheelSegment (line 758) restate their fields' names, so dropping them loses nothing.

@vjeux
vjeux force-pushed the uikit/f1-public-animator branch from 3bc9310 to c0b9848 Compare November 2, 2026 21:34
@vjeux

vjeux commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in c0b9848: the plain-JavaScript example names the card by its rank and suit and renames it on each deal, as the React example does. A new case in both examples, "names the dealt card by its rank and suit", fails on 3bc9310's plain-JavaScript example.

On the stack: #8676 has merged and this branch is now on main, so the diff holds only the animator. The description's "Pull request size" line covers this pull request's own commit: spec:AST-107 FR5 keeps the playing-cards example with the hooks. The two GestureHost comments are #8676's lines, so I've passed them to its author.

@cixzhang

cixzhang commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

/approve-spec c0b9848

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes: createAnimator and useAnimator match spec:AST-107 FR3–FR6 and their tests pass, but the plain-JavaScript example mounts its deck from a mount-only useEffect (AnimatorPlayingCardsVanilla.tsx:164-169). Please move that work to a stable callback ref on the wrapper, which React 19 calls on attach and whose returned cleanup runs on detach, and drop the two refs; the inline comment has the shape. The size is over 2,000 lines, but the factory, the hook, their docs and both examples ship together as one API under FR5 and FR6, so no split is needed.

[Automated review]

Comment on lines +164 to +169
useEffect(() => {
if (table.current == null || button.current == null) {
return undefined;
}
return mountDeck(table.current, button.current);
}, []);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mount runs once because the wrapper's elements attach, and it depends on no prop or state. A stable callback ref on the wrapper can do the same work and return its cleanup (Core's React peer is 19), which is the pattern the repository asks for when work is caused by an element attaching:

function mountTable(wrapper: HTMLDivElement | null) {
  if (wrapper == null) {
    return undefined;
  }
  const table = wrapper.firstElementChild as HTMLElement;
  const button = wrapper.querySelector('button') as HTMLButtonElement;
  return mountDeck(table, button);
}
// <div ref={mountTable} style={…}> with the table div and the button as plain children

That removes both useRefs and the Effect.

@vjeux
vjeux force-pushed the uikit/f1-public-animator branch from c0b9848 to 6d3d599 Compare November 2, 2026 22:21
@vjeux

vjeux commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in 6d3d599: the plain-JavaScript example mounts its deck from a stable callback ref on the wrapper, in your shape. React 19 calls it on attach and runs its returned cleanup on detach, so the Effect and both refs are gone. A new case, "takes its deck down when it unmounts", checks that the card is removed and the animator destroyed after an unmount.

@cixzhang

cixzhang commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

/approve-spec 6d3d599

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes: createAnimator and useAnimator match spec:AST-107 FR3–FR6, and the plain-JavaScript example now mounts its deck from a callback ref, but this head breaks Core once it merges into main.

  • main now carries #8190, whose packages/core/src/Slider/sliderTouchController.ts still imports createAnimator from ../motion/animator (line 32), so after this pull request's rename to createMotionAnimator the merged tree fails Core's type check (TS2305) and every Slider thumb drag throws createAnimator is not a function (line 1189); please merge main and rename that import and call.

The size is over 2,000 lines, but the factory, the hook, their docs and both examples ship together as one API under FR5 and FR6, so no split is needed.

[Automated review]

* ```
*/
export function createAnimator(
export function createMotionAnimator(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On main this rename misses one caller: #8190's packages/core/src/Slider/sliderTouchController.ts still imports createAnimator from this module (line 32) and calls it at line 1189. Git merges the two without a conflict, but the result fails Core's type check with TS2305, and the Slider's touch controller throws createAnimator is not a function on the first thumb drag (9 Slider tests fail on the merged tree; all 148 pass on main). Merging main and renaming that import and call to createMotionAnimator fixes it.

@cixzhang cixzhang added the ci:run A maintainer lets this outside contributor's pull request run CI (AST-030 FR17) label Nov 2, 2026
@cixzhang
cixzhang marked this pull request as ready for review November 2, 2026 22:39
@vjeux
vjeux force-pushed the uikit/f1-public-animator branch from 6d3d599 to 234dda6 Compare November 2, 2026 22:55
@vjeux

vjeux commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in 234dda6, rebased onto current main: #8190's sliderTouchController.ts now imports and calls createMotionAnimator. No other importer of the old internal name is left in the repository. Type check and the Slider, motion, animator and hook unit suites pass on the merged tree.

@cixzhang

cixzhang commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

/approve-spec 234dda6

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommending approval: createAnimator and useAnimator match spec:AST-107 FR3–FR6, and the Slider's touch controller now calls the renamed internal factory, so Core type-checks and the Slider tests pass on this head.

The pull request is over 2,000 lines, but the factory, the hook, their docs and both playing-cards examples ship together as one API under FR5 and FR6, so no split is needed. Springs that follow a theme's tempo and character (spec:AST-108 FR3) remain internal-animator work, as the description says.

[Automated review]

@vjeux
vjeux force-pushed the uikit/f1-public-animator branch from 234dda6 to 6921f57 Compare November 3, 2026 02:31
@vjeux

vjeux commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Updated in 6921f57, rebased onto current main. The four behaviors you asked an owner to settle are now decided and recorded in spec:AST-107 DEC-4, with FR1, FR3 and FR6 stating them. useAnimator now drops its values when its element unmounts or a node replaces it, so a remount or a new node starts from the element's resting values; two new cases fail on the previous head. Both playing-card examples also ignore a press on a leaving card, and a press on a card fading in moves only its position, each with a case that fails without it.

@github-actions github-actions Bot added the needs:spec-owner-review Current knowledge records await owner approval label Nov 3, 2026
@cixzhang

cixzhang commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

/approve-spec 6921f57

@github-actions github-actions Bot removed the needs:spec-owner-review Current knowledge records await owner approval label Nov 3, 2026

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested: the animator, its hook, the examples and the record update look right, but the internal rename breaks a caller that landed on main after this head was pushed.

  • packages/core/src/Switch/useSwitchTouch.ts (from #9636, now on main) still imports createAnimator from ../motion/animator and calls it three times, so with this pull request merged Core's type check fails and every Switch with a touch presentation throws createAnimator is not a function when it attaches; please merge main and rename that import, its three calls and the createAnimator key in the useSwitchTouch.test.ts mock to createMotionAnimator.

Non-blocking: the comment on REST in createAnimator.ts restates the values; a line saying a new or remounted element starts from them would keep only the reason.

[Automated review]

* ```
*/
export function createAnimator(
export function createMotionAnimator(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With main merged, packages/core/src/Switch/useSwitchTouch.ts:26 (from #9636) still imports createAnimator from this module and calls it at :288, :291 and :296. Core's type check then fails (TS2305) and a touch-presentation Switch throws createAnimator is not a function when it attaches. Please rename that import, the three calls and the createAnimator key of the vi.mock('../motion/animator') stand-in in useSwitchTouch.test.ts too.

};
}

/** Every channel at rest: in place, full size, upright and opaque. */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking (comment style): this restates the constant's values. Keep only the reason, for example /** A new or remounted element starts from these values. */.

The public animator (spec:AST-107 FR3, FR6): createAnimator(element)
over the internal animator, with animate, decay, set, get, getVelocity,
stop and destroy, from the package root; and useAnimator(ref), which
takes the element's ref like the gesture hooks, so a gesture hook can
share the element, and returns the animator. After every commit the hook
follows the element behind the ref, through the attach lifecycle the
gesture hooks share (useAttachment). The internal factory is renamed
createMotionAnimator, along with its callers in the Slider's and the
Switch's touch controllers and the pager's tests, so no internal factory
shares a public factory's name. Docs, a changeset, and playing-cards
templates in React and plain JavaScript, with their registrations. In
both templates the card is an image named by its rank and suit, renamed
on each deal; one card leaves at a time (a Discard press while a card
leaves deals no second card), a leaving card takes no press, and a press
on a card fading in moves only its position; and a drag the browser
cancels springs back. The plain-JavaScript example mounts its deck from
a callback ref on its wrapper, whose cleanup takes it down. The core API
report gains the animator's rows.

When the element behind useAnimator's ref unmounts, or a node replaces
it, the animator's values return to rest, so a remount or a new node
starts from the element's resting values. AST-107 records the owner's
decision on four edge behaviors of the public animator and pan gesture
(DEC-4): FR1 and FR6 state that a cancelled pan reports zero velocity,
how the pan factory's destroy and the hook's unmount report, and that an
opacity decay stops at 0 or 1; FR3 states the remount.
@vjeux

vjeux commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in 271fdeb, merged onto current main: useSwitchTouch.ts imports and calls createMotionAnimator, and its test's vi.mock('../motion/animator') stand-in carries that key. Before the rename, Core's type check failed with TS2305 and three Switch tests threw createAnimator is not a function; both pass now. The pager's tests, which landed on main since, are renamed too, and no other importer of the old name is left. The REST comment now keeps only its reason.

@cixzhang

cixzhang commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

/approve-spec 271fdeb

This branch was successfully deployed

1 active deployment
Preview — 271fdebf Deployed Nov 3, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:run A maintainer lets this outside contributor's pull request run CI (AST-030 FR17) CLA Signed This label is managed by the Meta Open Source bot. needs:design-review Affects visuals — Design should review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants