Repository navigation
chore(core): the touch motion tables register FR4's roles as internal motion roles - #8257
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
ecd60ce to
765a094
Compare
![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 needs an engineering owner's decision before it can be approved: theme motion roles (defineTheme({motion: {springs, durations, curves}}) writing --motion-<role> variables whose defaults live in the motion tables) and the new platform adaptation condition go beyond architecture:theme-tokens INV1 and spec:AST-012 FR2, and at this head the draft spec:AST-094 FR4 and DEC-1 still say touch motion is not themable. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request once the head carries the intended FR4 (the author may do so as an engineering owner), or by updating those records on main. These fixes are needed either way:
- The touch-motion docs and the
[feat]changeset say components read the roles, but nothing reads a--motion-*variable at this head. - Through
@astryxdesign/core/themealone, role validation never runs, so a misspelled or mis-grouped role id is accepted silently (expandMotionRoles.ts:140). - Add built-theme and duplicate-condition coverage for
platformrules, and rebase once #8052, #8053 and #8054 land so this carries only its own commit.
[Automated review]
| `${location}: a motion role is named by its table path, such as "sheet.present" or "springs.navigationPush".`, | ||
| ); | ||
| } | ||
| const registered = motionRole(id); |
There was a problem hiding this comment.
Through @astryxdesign/core/theme alone no motion table is registered when this runs, so motionRole(id) is always undefined: durations: {'springs.sheetPresent': 300} and a misspelled springs.sheetPresnt are both accepted and emitted. The refusal test passes only because expandMotionRoles.test.ts imports ../motion/uikit first. Register the shared tables where this validation runs and test without that import.
| }, | ||
| { | ||
| type: 'prose', | ||
| text: 'A theme writes one CSS variable per role it sets, `--motion-` followed by the path in kebab case: a spring as its duration and bounce (`0.6s 0.1`), a duration in milliseconds, a curve as `cubic-bezier()`. Components read the variable when a motion starts and fall back to the default when it is empty. Gesture thresholds, the delays that decide whether a touch scrolls or presses, and haptics are not motion roles. The `platform` condition matches iOS and iPadOS browsers (`ios`) or every other browser (`other`).', |
There was a problem hiding this comment.
At this head no component reads a --motion-* variable (nothing outside tests imports the motion tables), so "Components read the variable when a motion starts" and the section's opening sentence describe behavior that does not exist yet. Ship this section with the change that makes a component read the roles, or say here that none does yet.
| measures (0.55 for a scroll view and a sheet), and a release MUST pick its | ||
| rest point by projecting its velocity the way its UIKit counterpart does: | ||
| with a deceleration rate, or with a measured projection time. | ||
| - **FR4 — Physics are not themable in touch presentations.** A touch |
There was a problem hiding this comment.
This FR4, with DEC-1 at line 271, says touch physics are not themable: the opposite of the theme motion roles this pull request adds. The revised FR4 is only on the spec pull request; rebase onto it so the record and the code agree before owners decide.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
…to the motion-role animator # Conflicts: # packages/core/src/motion/README.md # packages/core/src/motion/roles.test.ts # packages/core/src/motion/uikit/timing.ts # packages/core/src/motion/uikit/uikit.test.ts
…plays its table value) into the experimental touch exports Four files both sides changed: coast.ts takes the animator's wording (the edge spring plays its table value); useAnimator.ts takes its registration line and keeps this side's note that a component may play its own measured spring; roles.test.ts takes the menu spring's motionRoleOf check beside this side's keyboard checks; resolveMotion.test.ts takes the animator's tests and keeps this side's coast test (scroll.bounce is no role).
…; a motion role plays its table value) into the presented start The animator's browser proof: its role check takes the inverted expectation (both boxes play the table's spring) and keeps this side's 0.26 px tolerance for a start at the commit.
…motion clock and Reduce Motion plan The motion README takes main's internal-roles sentence with this layer's animator; uikit.test.ts takes main's version (rows, docAbove) and keeps the check of values bound from an exported group, which timing.ts's Reduce Motion fade bindings need.
…nimator # Conflicts: # packages/core/src/motion/README.md # packages/core/src/motion/uikit/uikit.test.ts
…keyboard notification export
uikit/uikit.test.ts: main's leavesOf (a one-line export is a row, sourced
or not) keeps this branch's check of values bound from an exported group
(const {curve: move} = group). uikit/timing.ts merged cleanly.
|
Since you asked for my take on tokenizing touch motion: do as you want. The public motion-token set is your call, and we'll implement whichever set you choose. Until then touch motion stays internal and If it's useful as input, the measured touch values and the FR4 roles they map to are in #8502's tables: springs, and durations and curves. |
Maintainer impact
Touch presentations play springs, durations and curves from the measured UIKit tables. A few of these values are the motion roles
spec:AST-094FR4 names for their use (springs.settle,timing.stateChangeMs,timing.curves.move, …). This pull request gives those roles one internal registry. No public surface changes: themes don't set motion roles (theme tokens are never component-specific, and the public motion-token set is decided separately), and no shipped component reads the roles yet.Intended invariant
The registered roles are exactly the 17 FR4 names. Every other table value, a component's measured spring included, is an internal constant that plays as measured.
Change
registerMotionTablemakes a role only of the values a table lists inroles. The shared springs and timing tables list FR4's names; the scroll and navigation tables list none.SHEET_PRESENT_SPRING,SETTLE_SPRING, …), a literal a bundler can drop when nothing imports it, andspringscomposes them, so a component bundles only the springs it plays (architecture:component-style-authoringINV6). Registering the roles takes only FR4's twelve springs.--motion-springs-settle) that no public API writes. An empty property means the table's value.defineTheme({motion})keeps only the duration scale, as on main. A test locks the theme entry and the package root against a motion-role surface.[chore]changeset; the motion README androles.tsdescribe the internal roles.Evidence
@astryxdesign/core/themeor the root.Scope
Testing
pnpm exec vitest run --project ui packages/core/src/motion packages/core/src/theme: green.pnpm build,pnpm -F @astryxdesign/core typecheck, the node suites (stable-output goldens included) andpnpm check:repo: green.