Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
meta-cla
Bot
added
the
CLA Signed
48c0c4c to
65fb64b
Compare
![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: draft spec:AST-094 FR4 and DEC-1 say touch presentations read their motion from the UIKit table on every theme, while this pull request lets defineTheme({motion}) retune that motion and makes the animator play the theme's value. An engineering owner (the author may record it, as one) either amends FR4 and DEC-1 to admit motion roles and records approval by commenting /approve-spec <full head SHA> on the amended head, or keeps FR4, in which case the motion roles and the animator's theme resolution come out of this stack. Whatever the decision, please also:
- Rebase once the earlier pull requests in the stack merge, so this diff is only its own commit; it currently carries about 14,500 authored lines.
- Drop the new
platformadaptation condition, or amendspec:AST-012FR2 first, since that spec keeps the condition list closed. - Hold back the
astryx docs motiontouch-motion section, the template text and the[feat]changeset until a component actually reads a role. - Make
defineThemereject wrong-kind and unknown role ids without relying on the motion tables having been imported first.
[Automated review]
| * is everything else. Pair `ios` with `pointer: 'coarse'` for a touch | ||
| * profile, such as touch motion tuned per platform. | ||
| */ | ||
| platform?: 'ios' | 'other'; |
There was a problem hiding this comment.
spec:AST-012 FR2 keeps the when vocabulary closed (width, pointer, contrast, motion) and says future fields need an explicit addition to it. Neither AST-012 nor the draft AST-094 adds platform, so this field needs an AST-012 amendment first, or it should come out of this stack.
| `${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.
motionRole(id) is undefined unless a motion table module was imported earlier, and @astryxdesign/core/theme loads none. So in a real defineTheme call, durations: {'springs.sheetPresent': 300} and a misspelled id are both written without error, and the same input throws once a table is loaded. Loading the tables (or a static role manifest) here, and rejecting unknown ids, would make validation deterministic.
| watch(doc); | ||
| const scope = element.closest('[data-astryx-theme]') ?? doc.documentElement; | ||
| let cached = scopes.get(scope); | ||
| if (cached == null || cached.epoch !== epoch) { |
There was a problem hiding this comment.
epoch moves only when a MutationObserver callback runs, a microtask after the attribute change, so a theme switch followed by a motion in the same task still reads the old scope values. Calling observer.takeRecords() here, and bumping the epoch when it returns records, would close that window.
| }, | ||
| { | ||
| 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.
No component reads these variables at this head: nothing outside motion/ imports the animator or resolveMotion, and utils/pressGesture.ts keeps its own press timings. A theme author following this section sees no change. Could the section wait for the pull request that wires the first component?
…g running Landing a waiting Reduce Motion move on the node just detached rebuilt every channel the landing touched there, so an axis still moving (a coast on y while x waited in a dip) was baked onto the detached node again and promoted it, leaving an animation, timers and will-change behind. After the landing, the unmount now cancels what runs, clears the retire timers and demotes, before the target goes back to none. The test fails on the previous head.
…seudo-element support
The README's busy-main-thread paragraph and busyWindowProof's JSDoc said an undelivered frame is never a miss; when fewer than minJudgedFrames frames were delivered in the judged part, its span counts every frame. One clause in each says so.
…ose finds it The unmount kept the node of the first attach(null) in the task, so after attach(null), attach(next), attach(null) it landed a waiting Reduce Motion move on the node already replaced, and next stayed at its dip's opacity. A dispose right after attach(null) landed on no element, leaving the detached node at opacity 0. The latest detached node is now kept while its unmount is pending; the unmount and dispose both land there. Two tests fail on the previous head.
…ile it ran A page opened after startCompositorTrace and closed before stop() was still credited: its frames are in the browser-wide record, and neither check saw it. The record now watches for new pages from start to stop, in the page's own context and through the browser's target discovery in every context, and stop() throws if one opened. Negative controls: a page opened and closed mid-record, and one in another context, both make stop() refuse; alone, it records.
Inside a trace, waitForAnimation and watch.started() read the element through handles, which find no element rather than wait for one, so a wait on an element the page removed rejected with "no animation started on the element" instead of "the element is not in the document", unlike the untraced path. At its deadline, a traced wait with no element now rejects as missing. Traced twin of the missing-element self-test; it fails on the previous code. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
…rred unmount runs It read will-change right after attach(null), before the unmount's microtask, so it passed while that microtask could still promote the element and leave timers behind. It now awaits the microtask, then checks will-change and that no timer is left. It fails on meta#8521's earlier head (97436b2), before the unmount cleared what the landing rebuilt.
…pseudo-element support
…tor's browser proofs The motion README's Files table is the union: main's rows, with meta#8663's resolveSnapPoint.ts and selectPage.ts, plus this branch's, in main's order. # Conflicts: # packages/core/src/motion/README.md
There was a problem hiding this comment.
Changes are needed before this can be approved: resolveMotion returns each role's table value, as spec:AST-094 FR4 and DEC-11 require, but the diff still carries the unmerged stack beneath it, 5,740 authored lines against this pull request's own 384. Every carried copy matches its pull request's current head, so what remains is ordering: let #8067 and the pull requests beneath it land, then update this branch from main so the diff holds only its own commits. Please also add a changeset naming @astryxdesign/core for this pull request's own changes, since the changeset coverage check passes today only through the carried #8522 and #8067 changesets and fails over the own commits alone. The description below its update line still describes the withdrawn style read and an own size of 534 added lines; trimming it would help the next reader.
[Automated review]
The changeset coverage check passed only through the changesets this branch carries from the pull requests beneath it. This one names @astryxdesign/core for this pull request's own change: the animator plays each spring and curve, and a coast its edge spring, through the motion role the value plays, which is its table value while touch motion stays internal.
There was a problem hiding this comment.
Changes are needed before this can be approved: resolveMotion returns each role's table value, as spec:AST-094 FR4 and DEC-11 require, and the new changeset covers this pull request's own changes, but the diff still carries the unmerged stack beneath it, 5,747 authored lines against this pull request's own 391. Every carried copy matches its pull request's current head, so what remains is ordering: let #8067 and the pull requests beneath it land, then update this branch from main so the diff holds only its own commits.
[Automated review]
|
Posted by Cindy's agent on her behalf: closed and reopened only to restart this PR's CI on the current workflow after tonight's queue-wide cancel; nothing in the PR changed. |
Depends on #8067 (#8257 is merged). Review its own commits; the merges bring them forward.
Why
Each touch motion names its spring or curve at the call site. Any later decision about how a kind of motion should feel, such as a theme's
tempoandcharacter(#8677), would have to change every one of those call sites.What led to it
This comes from the work to make Astryx's touch components behave as native iOS ones do (sheets, menus, gestures and their motion).
spec:AST-094FR4 names the motion roles those components play, and #8257 registers exactly those roles by their table paths.Options and tradeoffs
Why this choice
The animator is the one place every touch motion passes through, so resolving there changes nothing at call sites; #8677 adds the theme's knobs at that same place (
spec:AST-108FR3).What changes
resolveMotion(element, role)returns a role's value: its table value while touch motion stays internal. A value that is no role, such as a spring written inline, comes back unchanged.How to check it
pnpm exec vitest run packages/core/src/motion/resolveMotion.test.ts: springs, curves and durations play their table values whatever a scope sets.Evidence
Scope