Repository navigation
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
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval is on hold until an engineering owner rules on the draft spec:AST-094, the only authority this change cites, including whether @astryxdesign/core/motion/experimental and @astryxdesign/core/gestures/experimental count as experimental boundaries under spec:AST-017 FR21 and whether its compact-touch rule may change the released adaptive presentation of DropdownMenu, ContextMenu, MoreMenu, Selector and MultiSelector. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Independently of that decision, two changes are needed:
- The four
stability: 'experimental'hook docs appear in stableastryx searchresults and add ShadCN routes, whichspec:AST-017FR27 rules out without an opt-in and which fails the experimental-isolation snapshots and the ShadCN route lock in CI. - Against
mainthe diff is 24,002 authored lines across 140 files rather than the five-file refresh commit, so it should be rebased ontomainonce the predecessor pull requests land.
[Automated review]
| export const docs = { | ||
| name: 'useAnimator', | ||
| displayName: 'useAnimator', | ||
| stability: 'experimental', |
There was a problem hiding this comment.
stability: 'experimental' does not keep this hook out of stable outputs: astryx search experimental now returns it, useGestureRecognizer, usePressTracker and useTouchPresentation as ordinary results with astryx hook commands, and the docsite generates ShadCN routes for all four that internal/shadcn-registry/routes.lock.json does not hold. spec:AST-017 FR27 requires experimental docs on marked routes and stable output unchanged without an opt-in, and CI's experimental-isolation.test.mjs and the ShadCN route lock fail on it. Keep these docs out of stable search, hook listings and the ShadCN registry, or drop them until a marked experimental route exists.
| // and large touch displays stay anchored, phones in either orientation get | ||
| // the sheet. The query is the shared touch presentation's | ||
| // (utils/touchPresentation.ts). | ||
| export const COMPACT_TOUCH_PRESENTATION_QUERY = COMPACT_TOUCH_QUERY; |
There was a problem hiding this comment.
This changes what the released adaptive value opens on DropdownMenu, ContextMenu, MoreMenu, Selector and MultiSelector, and what their default will open once CLN-0020 makes adaptive the default: a phone in landscape wider than 768px now gets the bottom sheet, and a 744-768px touch tablet in portrait the popover, where ContextMenu's and MoreMenu's released docs promised the sheet at 768px and below. component:Selector DEC-1 says adaptive keeps its width-and-pointer query, and the new rule comes from draft spec:AST-094 FR13, still pending owner review, so it waits on an engineering owner's ruling, including whether it may ship as a patch [feat].
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewComponent 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: 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 |
c4db3cc to
ebb162a
Compare
There was a problem hiding this comment.
Approval is on hold until an engineering owner rules on the draft spec:AST-094, the only authority this change cites, including whether @astryxdesign/core/motion/experimental and @astryxdesign/core/gestures/experimental count as experimental boundaries under spec:AST-017 FR21 and whether its compact-touch rule may change what the released presentation="adaptive" opens on landscape phones and 744-768px touch tablets, as a patch. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Independently of that ruling, these changes are needed:
- The four
stability: 'experimental'hook docs still enter stableastryx searchoutput and ordinary ShadCN routes, whichspec:AST-017FR27 rules out without an opt-in, and rewriting thesearch-experimentalgoldens androutes.lock.jsonhides that from CI instead of isolating the docs. - Against
mainthe diff is 25,257 authored lines across 143 files rather than the five-file refresh commit, so it should be rebased ontomainonce the predecessor pull requests land. - AST-094 FR7 cites
spec:AST-090, which does not exist in the repository, and the description lacks the spec-writing rubric results for the new record.
[Automated review]
| "data": { | ||
| "query": "experimental", | ||
| "matchCount": 3, | ||
| "matchCount": 7, |
There was a problem hiding this comment.
This golden is the stable astryx search experimental output with no opt-in, and the test's header (packages/cli/test/experimental-isolation.test.mjs:35-39) calls a golden diff that names experimental the leak it exists to catch. Rewriting it to 7 results, with useAnimator, useGestureRecognizer, usePressTracker and useTouchPresentation as ordinary hook results, makes CI accept what spec:AST-017 FR27 rules out ('An omitted opt-in MUST leave stable defaults, output, routes, data, and behavior unchanged'); the four hook-use-* entries added to internal/shadcn-registry/routes.lock.json do the same for routes. Keep stability: 'experimental' hook docs out of stable search, hook listings and ShadCN generation, and restore this golden, its text twin and the route lock.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
No spec:AST-090 record exists on main or at this head, so an owner asked to rule on this departure cannot read the FR1 and FR10 it departs from. Cite the record where those claims live, or drop the note until AST-090 lands; line 235 repeats the citation.
ebb162a to
59ebc50
Compare
59ebc50 to
384295d
Compare
github-actions
Bot
added
the
needs:design-review
There was a problem hiding this comment.
Approval is on hold until an engineering owner rules on the draft spec:AST-094, the only authority this change cites: whether its FR1-FR3 govern the UIKit constants table, whether @astryxdesign/core/motion/experimental and @astryxdesign/core/gestures/experimental count as experimental boundaries under spec:AST-017 FR21, and which record governs the defineTheme({motion: {springs, durations, curves}}) touch motion roles (which AST-094 FR4 rejects) and the platform adaptation condition (outside the closed when vocabulary of spec:AST-012 FR2). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. The adaptive compact rule now matches the current spec:AST-012 FR9 and needs no ruling. Independently of the ruling, these changes are needed:
- The four
stability: 'experimental'hook docs still enter stableastryx searchoutput and ShadCN routes, whichspec:AST-017FR27 rules out without an opt-in, and the rewrittensearch-experimentalgoldens androutes.lock.jsonhide that from CI. - The touch-presentation resolver reads the default
mdpoint, so a theme that overridesmddoes not get the FR9 compact rule the changeset promises. - Stable
astryx docs motionand the[feat]changeset describe motion roles that no component plays yet, and the changeset's example roletiming.reducedMotionCrossFadeMsis no longer a role. - AST-094 FR8 asks for more motion under Reduce Motion than
design:reduced-motionDR2 and DR4 allow, FR7 cites aspec:AST-090that does not exist, and the description lacks the record's writing rubric results. - Against
mainthe diff is 28,933 authored lines across 189 files, including an unrelated@astryxdesign/buildoption; rebase ontomainonce the predecessor pull requests land.
[Automated review]
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [feat] Themes can retune touch motion. `defineTheme({motion})` takes `springs`, `durations` and `curves` keyed by motion role, the table path of a value touch presentations play (`springs.sheetPresent`, `timing.reducedMotionCrossFadeMs`), at the root or in an adaptation rule. A role the theme leaves out keeps its measured default, and the duration scale (`fast`, `medium`, `ratio`) is optional when a theme sets only roles. Adaptation rules gain a `platform` condition, `ios` or `other`. `astryx docs motion` lists every motion role with its CSS variable and default, and the theme authoring guide and template describe the new fields. |
There was a problem hiding this comment.
timing.reducedMotionCrossFadeMs is not a motion role at this revision: timing.ts:158 excludes it under design:reduced-motion DR4, and reduced-motion-record.md says it 'is no longer a theme role'. expandMotionRoles still accepts the id without error, so a theme author who follows this release note sets a variable nothing reads. Use a role that exists, such as navigation.transitionMs.
| export const COMPACT_QUERY = DEFAULT_COMPACT_DEFINITION.query; | ||
|
|
||
| /** Both: presentations such as a sheet in place of a popover. */ | ||
| export const COMPACT_TOUCH_QUERY = | ||
| DEFAULT_COMPACT_DEFINITION.coarsePointerQuery; |
There was a problem hiding this comment.
spec:AST-012 FR9 makes compact 'width … below the effective md point', and its verification lists 'overridden md'. These constants freeze the default 768px, and useAdaptivePresentation and the drill-in read them, so a theme whose adaptations.widthBreakpoints.md is 900 still gets the popover on an 800px coarse-pointer screen, against the changeset's 'narrower than the theme's md point'. Build the query from compactDefinition() over the active theme's points (AppShell reads them through useThemeDefinition()), and add the overridden-md case.
| - **FR8 — Reduce Motion substitutes, never jumps.** Under | ||
| `prefers-reduced-motion: reduce`, a touch presentation MUST behave as its | ||
| UIKit counterpart does with Reduce Motion and Prefer Cross-Fade Transitions | ||
| on: a transition that travels becomes a cross-fade carrying the same change | ||
| of state. An entrance lands in place and fades in, an exit fades out in | ||
| place, and any other move fades through. A navigation push or pop | ||
| cross-fades over 0.5 s on the ease-in-out curve; a sheet, a full-screen | ||
| modal or an alert fades on its own presentation spring and drops its travel | ||
| and scale; menus open and close on UIKit's Reduce Motion spring. Motion that | ||
| follows a finger, such as a throw, MUST keep its motion. **Proposed; pending | ||
| owner review:** this is the minimally moving form the draft `design:motion` | ||
| DR5 asks for, for touch presentations (owners: ernestt, cixzhang); the | ||
| alternative is UIKit's Reduce Motion alone, which keeps the slides of | ||
| navigation, sheets and modals. |
There was a problem hiding this comment.
FR8 asks for a 0.5 s navigation cross-fade, sheets fading on their presentation spring, and a throw that keeps its motion. The current design:reduced-motion DR2 requires an opacity-only fade of 200 ms or less on a coarse pointer, a touch drag's release included, and DR4 forbids any way to keep travel or a longer animation; this stack's own animator already follows DR2 at 200 ms. Align FR8 with that record, or name the departure for its owners.
| content: [ | ||
| { | ||
| type: 'prose', | ||
| text: 'On a touch screen, sheets, alerts, menus, navigation and scrolling play the springs, durations and curves measured from iOS. Each value is a motion role named by its table path, such as `springs.sheetPresent` or `timing.curves.easeInOut`. A theme retunes a role through `motion.springs`, `motion.durations` or `motion.curves`, at its root or in an adaptation rule; a role it leaves alone keeps its measured default.', |
There was a problem hiding this comment.
No component reads a motion role at this revision: outside motion/ only theme/expandMotionRoles.ts imports the role registry, and nothing plays the animator. A theme author who sets springs.sheetPresent or refreshControl.endMs on the strength of this sentence sees nothing change. Say that the roles apply to touch presentations that play them, or hold the section until one does.
| * | ||
| * @default 'warn' | ||
| */ | ||
| undefinedVariables?: 'warn' | 'error'; |
There was a problem hiding this comment.
undefinedVariables is a new public astryxStylex option, and the warning it controls now runs in every production build by default; no record governs it, the pull request does not mention it, and its changeset files it as [fix]. The cross-file defineConsts resolution beside it restores spec:AST-080 FR7; move the option to its own pull request and file it as a feature there.
| * 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 lists the when fields as width, height, pointer, contrast and motion, and says 'Future environment fields require explicit additions to this closed vocabulary'. platform needs that addition from an engineering owner before it ships as stable theme API; it is part of the open owner question.
384295d to
29bea1f
Compare
29bea1f to
10a2336
Compare
meta#8076's new head carries main 70ea859 with meta#8073's writer of the press attribute on main's press controller, and its README names both writers. utils/pressFeedback.ts keeps that controller with this line's own changes on it: the pointer, drag, scroll and visibility listeners through the shared input stream (meta#8356) and touchcancel only while the primary pointer is coarse (meta#8270); the key path stays at the window's bubble phase, so a Space press the page cancels paints nothing. usePressTracker stays in its own module, one tracker for a component's life (meta#8315), so meta#8073's rebuilt-tracker hook and its test are not taken. The harness files are meta#8076's.
…s, and the action-group tracker writes its own The header and writePress called it the one writer of the press attribute, but main's action-group tracker writes its groups' presses itself. They now say what holds, as meta#8076's gestures README does.
…he press attribute The release note said every press painter writes data-astryx-press through one internal writer, but the action-group tracker (hooks/actionGroupTracker.ts) writes its groups' presses itself, as the gestures README and utils/pressFeedback.ts already say. The note now says paintPress writes every press it tracks and the action-group tracker writes its own.
…llowance under every primary pointer The useMenuPress record's FR9 puts the trigger's held finger under spec:AST-094 FR29 under every primary pointer (FR43), so a finger on a touchscreen laptop may drift 13 pt before the open, as on a phone. The trigger read (pointer: coarse) as its press started and took the long press's 10 px where the pointer was fine. Three cases fail until it does not: a 12 pt drift opens under a fine pointer too, a (12, 12) settle opens there, and nothing reads a pointer query, the trigger's press included.
…lowance under every primary pointer The useMenuPress record's FR9 governs the trigger's held finger by spec:AST-094 FR29 under every primary pointer (FR43). The trigger now hands the long press FR29's 13 px drift allowance whatever the primary pointer, so it no longer reads (pointer: coarse) as its press starts, and the menus drop their coarsePointer import. ContextMenu keeps the long press's own allowable movement. The changeset says the same.
Strict ESLint reported six unused bindings in files this pull request carries: the host's HAPTIC_SWITCH_ATTRIBUTE import, two unused imports in the pan's tests (scrollProbe, ScreenEdgePanGestureRecognizer), the swipe test's drift, the presented-hit-testing test's gestures, and a GestureHost test's before, which now asserts what it was read for: a second finger's moves leave the pan where it was.
…eal angular speed The release velocity of a rotation whose listener sets rotation = 0 on each change, at 2 degrees every 16 ms, read 4.363 rad/s against the 2.182 rad/s of the same stream without a re-base: the test asserted only more than 1.5. It now asserts the 2.18 rad/s (spec:AST-100 FR2), and fails.
The rotation's velocity tracker samples the raw unwrapped angle, which setting rotation does not move, so the setter's shift of the samples read each re-base as extra turn and doubled the release velocity of a listener that re-bases on every change. The setter no longer touches the tracker.
…mates while it runs The proof read the element's animations 230 ms in, after the dip had ended, and asserted only that every animated key is opacity, which an empty list passes: with the cross-fade removed from the plan (the move still landing), the proof still passed. It now reads the keyframe keys right after the call starts and asserts they include opacity and nothing else; the same mutant fails it. Chromium and WebKit pass. (F1's in-flight read from c648034.)
…roller, and its claimed moves are cancelled under pointer events Two comparisons stop at the shadow boundary, each failing a case: - once the host's listener on a recognizer's inner node has seen the finger, isFingerDownIn reads it as outside a light-DOM scroller around that root, so a scroll under it is not the finger's (FR20); - under inputMode 'pointer-events', a claimed pan on a shadow host cancels none of its finger's moves over the inner content, since the finger's target is the inner node and its touches reach the host retargeted (FR19).
…the flat tree isFingerDownIn and the pointer-events finger match compare with containsAlongFlatTree, as the scroll watch already does: a finger on content inside an open shadow root is down in the scroller around that root, and its touches, retargeted to the shadow host, match its pointer finger, so a claimed pan cancels its moves there (spec:AST-094 FR19, FR20).
…achments asks, and only then The host let the last attachment decide whether an element is an action group (useGestureRecognizer always passes the option), and never recomputed it at a detach: a plain attachment after the group's turned the group off, and a group's attachment released left a recognizer that never asked taking presses under a fine pointer. Fails until the host counts the attachments that ask.
…ents asks The host counts the attachments that pass actionGroup: true and keeps the element a group while one remains, recomputed at each detach. An attachment that passes false, as useGestureRecognizer does by default, no longer turns a group off, and a group's last asking attachment taking its leave ends the group.
… that content The presented hit test takes the document's elementFromPoint, which answers a shadow host for anything inside its open root, and compares with Node.contains, which stops at the boundary: a recognizer on a control inside a moving sheet's shadow root gets no finger, and the tap's click goes to the host. Fails until the hit test descends into open roots and compares along the flat tree.
Where a moving element is drawn away from its layout box, the host hit-tests what is drawn under the finger with the document's elementFromPoint, which answers a shadow host for anything inside its open root. The hit test now descends through each open root's own elementFromPoint and compares with the movers along the flat tree, so a recognizer on a control inside a moving sheet's shadow root gets its finger and the tap clicks that control.
…nt lets go The host's touch-action rules sit in an adopted sheet or a style element per document or shadow root, and were never removed, so a page whose last recognizer detached kept them, against spec:AST-094 FR24 (nothing touch-related remains after the last instance unmounts). Fails until a root's rules go with its last marked element.
…th its last marked element The touch-action writer records which elements it marked in each document or shadow root, and when the last one is unmarked (its last recognizer detached) it removes that root's adopted sheet or style element, so nothing touch-related remains after the last instance unmounts (spec:AST-094 FR24). A later mark writes the rules again; an element marked in another root first lets go of the old one.
… the inline style it reads - The motion registry, the host's engine gate and the README name the presented hit test as an engine workaround (spec:AST-013 FR6) with its review condition (re-measure on each iOS release; remove it once iOS WebKit hit-tests a compositor-moved element where it is drawn) and its regression fixture. - While a curve runs the animator holds the value it starts from inline, so presentation.ts and the README no longer say the inline style holds the end.
…core/internal/touch Core ships the internal entry @astryxdesign/core/internal/touch, which re-exports the gesture primitives for Astryx's own packages, and the Lab-import guard fails a Lab import of core outside core's exports (architecture:react-component-runtime INV13). The README and the barrel's header named internal workspace imports and said nothing here is exported; they now name that entry.
…ill needs the element, and the listeners that cancel The release note, the stream's header, the host's comment and the README said the stream is one passive listener per event type and that a document-capture stopPropagation no longer starves the recognizers. Such a stop never hid a listener on the same node, and some subscriptions cancel. They now say what changed: an app's window-capture stopPropagation, or a document-capture stopImmediatePropagation registered before core, no longer hides a touch; a recognizer that must claim still needs the touch to reach its element; the gesture host's touchend, click and contextmenu cancel while a claimer is attached and the page hold's touchmove while it holds; and the stream stays ahead of an app's window-capture listener only until a type's listener is re-added. The README's list of blocking touchmove listeners gains the claimed touch's target and the document while a claiming element moves.
…default it moves
The {stylexOptions} form of astryxStylex now passes StyleX
treeshakeCompensation: true unless the caller sets it. spec:AST-017 FR3
asks the release note to name the old and new default and the option
that restores the old one; it now does.
…er and for the two stops the input stream fixes - A still finger with sub-pixel jitter over a scroll view keeps its tap and scrolls nothing (spec:AST-094 FR18–FR22). - A tap is still recognized under an app's window-capture stopPropagation, and under a document-capture stopImmediatePropagation registered before core's: the recognizers hear touches on the window's capture phase. Chromium and WebKit pass.
…s once Where nothing that claims is attached, the host's touchend listener is passive and cannot cancel the engine's own click after a retargeted tap clicks what is drawn. The shared click guard swallows that late click whatever is attached; this case pins it, as asked on meta#8293.
…esture host's subscriptions The gesture host subscribes no click: the one cancelling click subscriber is the click guard's, added at a window's first arm and kept. The release note, the stream's header and the gestures README listed click with the host's touchend and contextmenu; they now name the guard's own.
There was a problem hiding this comment.
Changes requested: the refreshControl values follow spec:AST-094 FR1 and FR2 and can stay out of the motion role table while nothing plays them, but against main this pull request still carries about 27,200 lines of open predecessor work around the 244-line refresh change. Please retarget it onto #8360's branch, or rebase once the predecessors land, so only the refresh change remains.
- In the carried stack,
GestureHoststays inert under a fine primary pointer apart from per-event opt-outs (GestureHost.ts:12-22), while AST-094 FR19 and FR43 andarchitecture:react-component-runtimeINV13 now have gestures attach for every pointer; that fix belongs in #8336. PressTrackeris a new class (PressTracker.ts:179) where INV13 asks for acreate…factory.- The header at
refresh.ts:42-44would stay true after the Lab control lands if it said 'not a role until a refresh-control record measures it (DEC-5)'.
[Automated review]
| * ONLY WHERE THE PRIMARY POINTER IS COARSE | ||
| * | ||
| * - Recognizers attach, and the host registers listeners, only while the | ||
| * primary pointer is coarse (`spec:AST-094` FR19, FR24; coarsePointer.ts). | ||
| * Where it is fine, an attach is recorded but nothing is installed: no | ||
| * document, window or element listener, no `touch-action` or other style, | ||
| * nothing delivered, so a finger on a touchscreen laptop gets the | ||
| * browser's defaults. The host follows the query: turning coarse installs | ||
| * it and styles every registration; turning fine cancels every touch in | ||
| * flight and removes it all. A development warning names the first attach | ||
| * under a fine pointer. |
There was a problem hiding this comment.
spec:AST-094 on main now says the opposite of this rule. FR19: 'Recognizers attach for every pointer, with FR43's exceptions'; FR43: 'Gestures attach for every pointer, a mouse, a pen, and a finger, not only a coarse primary pointer'; DEC-21 rejects 'keeping gestures and press tracking touch-only'; and architecture:react-component-runtime INV13 says 'The gesture host attaches for every pointer, with no touch-only query'. Under a fine primary pointer a component built on this host gets no gesture from any input, a finger included, and no mouse or pen press through a press tracker, unless it opts out per event or is an action group. Suggest attaching for every pointer, keeping FR43's exceptions and FR24's fine-pointer budget (own element, page listeners only during a press), and turning GestureHost.test.ts:3037-3132 around. This belongs in #8336.
| return tap.numberOfTapsRequired === 1 && tap.numberOfTouchesRequired === 1; | ||
| } | ||
|
|
||
| export class PressTracker implements TouchConsumer { |
There was a problem hiding this comment.
architecture:react-component-runtime INV13 keeps classes to 'The recognizer family (its base, the arena, and the host)' and says 'Every other module, and every public layer, is built from create… factories', listing only the velocity tracker as a known gap. A createPressTracker factory returning the same handle would keep this module inside that rule.
|
On the review of 1125110:
|
Depends on #8360 (the gesture foundation, which carries #8076). Review only this PR's change: vjeux/astryx@045f666...1125110
Why
The lab RefreshControl should feel like iOS: how far the content stretches, when the refresh fires, where it rests and how it leaves. That takes UIKit's measured numbers.
What led to it
This is part of the touch-presentations program (
spec:AST-094), whose motion comes from one table measured from UIKit on iOS 26.2, each value with its source (FR1, FR2). The lab RefreshControl reads every motion value from that table, and UIRefreshControl's were missing, so I measured them on the iOS 26.2 simulator and added them inmotion/uikit/refresh.ts.Options and tradeoffs
scroll.bounce, which it reuses.Why this choice
Each value is what the simulator showed, so the control matches iOS. The two the simulator couldn't pin down (the spinning alpha step and the trigger spring's damping clamp) are marked as guesses, each with a measurement task in
MEASUREMENTS.md. They stay internal to core: nothing is exported or themeable, so callers see no change.How to check it
packages/core/src/motion/uikit/refresh.ts: each value says where it was measured; each guess names its task.Evidence
Stack (merge in this order): #8360 gesture foundation (carries #8076) → this PR (refresh values) → #8170 pull and state machine → #8171 useRefreshControl (also needs #8098) → #8175 RefreshControl.