Repository navigation
feat(core): createPanGesture and usePanGesture report a drag as values (spec:AST-107 FR1, FR6) - #8655
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:design-review
|
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.
This review is on hold for an owner decision. The public createPanGesture and usePanGesture rely on spec:AST-107, which exists only in open #8600 and is not on main, where spec:AST-017 FR21 and architecture:react-component-runtime INV13 keep gesture modules internal. An engineering owner can approve this public pan for this exact head by commenting /approve-spec <full head SHA> (the author may do so as an engineering owner), or the PR can wait for #8600 to land. That decision should also settle whether the pan reports mouse and pen drags where the primary pointer is fine; today it reports nothing there. Independent of that decision, these need changes:
- Land #8076 first (or retarget onto it), so this diff is only the pan: it now carries 16,566 authored lines, including a copy of #8076 that lacks its four newest fixes.
usePanGesturecallsonPanwithcancelledfrom its layout effects when the component unmounts oraxischanges during a drag, whicharchitecture:react-component-runtimeINV2 rules out.- The hook's docs should match what ships. They promise mouse and pen drags, point to
useDragToSnapPointsanduseAnimator(neither can be imported), and say a scroll that starts on the element stays a scroll, which a pan withoutaxisprevents.
[Automated review]
| if (attached?.element === element && attached?.axis === axis) { | ||
| return; | ||
| } | ||
| attached?.gesture.destroy(); |
There was a problem hiding this comment.
destroy() here, and again in the unmount cleanup at line 82, cancels a drag in progress, and the recognizer listener then calls the caller's onPan({state: 'cancelled'}) from inside the layout effect. architecture:react-component-runtime INV2 keeps consumer callbacks out of Effects, and useGestureRecognizer in this same diff suppresses exactly this case with isSyncingRef and isReleasingRef. Could the hook do the same, and add tests for an unmount and an axis change during a drag?
| ], | ||
| usage: { | ||
| description: | ||
| 'Reports a drag on an element as values, for a surface the product moves itself: a card that follows the finger, a drawer pulled by its handle. The drag claims its touch at UIKit’s 10 pt, as `UIPanGestureRecognizer` does, so a tap or a scroll that starts on the element stays a tap or a scroll, and a press or a long press is never reported as a pan. The hook is a thin layer over `createPanGesture`, the same API for code without React. A drag has no keyboard or assistive-technology path of its own: whatever the drag does needs another way to do it, such as buttons or arrow keys (WCAG 2.5.7), and `useDragToSnapPoints` provides `settleTo` for that. The pan moves nothing itself, so Reduce Motion applies to the motion you drive from it: animate with `useAnimator`, which plans every animation for Reduce Motion. Where a component already offers the behavior, use its prop instead: BottomSheet’s `snapPoints`, Carousel’s `hasSnap`.', |
There was a problem hiding this comment.
Three statements here don't match the head. useDragToSnapPoints does not exist, and useAnimator is internal (motion/useAnimator.ts is not exported), so neither the keyboard path nor the Reduce Motion guidance can be followed; relatedHooks at line 93 lists both, and the docsite's MCP route serves that list. A pan without axis gives the element touch-action: pinch-zoom, so a scroll that starts on it does not stay a scroll. And the page doesn't say that nothing is reported where the primary pointer is fine.
| * What drove the drag: a finger, a mouse or a pen. A caller that animates | ||
| * the release can tell a touch's from a pointer's. | ||
| */ | ||
| pointerType: 'touch' | 'mouse' | 'pen'; |
There was a problem hiding this comment.
pointerType promises a finger, a mouse or a pen. But the gesture host listens for nothing where the primary pointer is fine (GestureHost.ts shouldListen), so on a desktop or laptop no drag of any kind is reported, and the pan's unit tests all run under coarsePrimaryPointer(). Whichever way the owner decides, the JSDoc, the hook page and the changeset should say what happens there.
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| DateTimeInput | 186.0 kB (+462 B) · 17.2 kB CSS (0) | — | — · — · — |
| DateInput | 164.0 kB (+273 B) · 15.5 kB CSS (0) | — | — · — · — |
| PowerSearch | 260.4 kB (+215 B) · 22.6 kB CSS (0) | — | — · — · — |
21 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| lab/Comments | 273.8 kB (+140 B) · 2.5 kB CSS (0) | — | — · — · — |
| charts/ChartTooltip | 146.0 kB (+1 B) · 423 B CSS (0) | — | — · — · — |
| lab/CommentComposer | 109.0 kB (+1 B) · 1.9 kB CSS (0) | — | — · — · — |
| DateRangeInput | 75.9 kB (0) · 11.2 kB CSS (0) | — | — · — · — |
| DrawerHeader | 49.9 kB (0) · 11.6 kB CSS (0) | — | — · ✓ · ✓ |
| lab/ChatEmojiPicker | 69.1 kB (0) · 773 B CSS (0) | — | — · — · — |
| lab/ChatReactionBar | 69.6 kB (0) · 1.1 kB CSS (0) | — | — · — · — |
| lab/ChatTypingIndicator | 25.1 kB (0) · 612 B CSS (0) | — | — · — · — |
| lab/DiffViewer | 73.1 kB (0) · 2.1 kB CSS (0) | — | — · — · — |
| lab/EmojiPicker | 72.5 kB (0) · 722 B CSS (0) | — | — · — · — |
| lab/ListInput | 85.5 kB (0) · 1.6 kB CSS (0) | — | — · — · — |
| lab/ThreeDChart | 5.0 kB (0) · 20 B CSS (0) | — | — · — · — |
| lab/TransferList | 80.0 kB (0) · 1.7 kB CSS (0) | — | — · — · — |
| lab/TransferListSelector | 87.1 kB (0) · 1.9 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditor | 283.7 kB (0) · 2.6 kB CSS (0) | — | — · — · — |
| richtext/RichTextView | 264.0 kB (0) · 2.3 kB CSS (0) | — | — · — · — |
| lab/CommentPin | 48.5 kB (−1 B) · 1.2 kB CSS (0) | — | — · — · — |
| charts/Chart | 168.8 kB (−2 B) · 494 B CSS (0) | — | — · — · — |
| Calendar | 57.7 kB (−3 B) · 9.3 kB CSS (0) | — | — · — · — |
| lab/CommentThread | 268.3 kB (−261 B) · 2.4 kB CSS (0) | — | — · — · — |
| Drawer | 38.0 kB (−3.4 kB) · 8.2 kB CSS (−29 B) | — | — · ✓ · ✓ |
Package size:
- @astryxdesign/cli 3.37 MB packed (+2.8 kB); no new dependencies
- @astryxdesign/core 6.57 MB packed (+7.3 kB); no new dependencies
API changes: core +11 · ~0 · −0
Committed API report behind this build: cli 9 rows, core 4 rows; run pnpm api:report.
11 API changes (advisory)
| Package | Where | Signature | Usage | |
|---|---|---|---|---|
| + | core |
. · Hooks · hook usePanGesture |
hook usePanGesture: (ref: RefObject<Element | null>, options: PanGestureOptions) => void |
import {usePanGesture} from '@astryxdesign/core'; usePanGesture(…) |
| + | core |
. · Functions · function createPanGesture |
function createPanGesture: (element: Element, options: PanGestureOptions) => PanGesture |
import {createPanGesture} from '@astryxdesign/core'; createPanGesture(…) |
| + | core |
. · Types · interface PanGesture |
interface PanGesture |
import type {PanGesture} from '@astryxdesign/core' |
| + | core |
. · Types · interface PanGestureEvent |
interface PanGestureEvent |
import type {PanGestureEvent} from '@astryxdesign/core' |
| + | core |
. · Types · interface PanGestureOptions |
interface PanGestureOptions |
import type {PanGestureOptions} from '@astryxdesign/core' |
| + | core |
. · Types · type PanGestureState |
type PanGestureState = 'began' | 'changed' | 'ended' | 'cancelled' |
import type {PanGestureState} from '@astryxdesign/core' |
| + | core |
./hooks · Also exported from an earlier entry point · PanGesture |
PanGesture (.) |
import {PanGesture} from '@astryxdesign/core/hooks' |
| + | core |
./hooks · Also exported from an earlier entry point · PanGestureEvent |
PanGestureEvent (.) |
import {PanGestureEvent} from '@astryxdesign/core/hooks' |
| + | core |
./hooks · Also exported from an earlier entry point · PanGestureOptions |
PanGestureOptions (.) |
import {PanGestureOptions} from '@astryxdesign/core/hooks' |
| + | core |
./hooks · Also exported from an earlier entry point · PanGestureState |
PanGestureState (.) |
import {PanGestureState} from '@astryxdesign/core/hooks' |
| + | core |
./hooks · Also exported from an earlier entry point · usePanGesture |
usePanGesture (.) |
import {usePanGesture} from '@astryxdesign/core/hooks' |
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
There was a problem hiding this comment.
This review is on hold for an owner decision: the public createPanGesture and usePanGesture rely on spec:AST-107, which exists only in open #8600, while spec:AST-017 FR21, architecture:react-component-runtime INV13 and spec:AST-002 DEC-8 on main keep a gesture module internal until its contract is current. An engineering owner can approve this public pan for this exact head by commenting /approve-spec <full head SHA> (the author may do so as an engineering owner), or the PR can wait for #8600 to land; that decision should also settle pointerType and whether the pan may ship reporting nothing where the primary pointer is fine. Independent of that decision, these need changes:
- Land #8076 first (or retarget onto it), so this diff is only the pan: it now carries 16,696 authored lines, all but 904 of them from #8076.
usePanGesturecallsonPanwithcancelledfrom its layout effects when the component unmounts oraxischanges during a drag, whicharchitecture:react-component-runtimeINV2 rules out; the internal gesture hooks in this diff already suppress that case.- The hook's docs should match what ships: they describe mouse and pen drags without saying that nothing is reported under a fine primary pointer, point to
useDragToSnapPointsanduseAnimator(neither can be imported), and say a scroll that starts on the element stays a scroll, which a pan withoutaxisprevents.
[Automated review]
341f29e to
32843e1
Compare
|
Rebased onto main at 32843e1. The pan's code is unchanged. Drawer now imports its three hooks from their own modules, as BottomSheet and Lightbox do: through the hooks entry, the new export pulled the gesture host into Drawer's static graph, which |
There was a problem hiding this comment.
Recommending approval. createPanGesture and usePanGesture match the public gesture-hooks spec: each reports a drag as {state, translation, velocity} under a finger, a mouse and a pen, with a docs page and a playing-card example. The four edge behaviours (zero velocity on cancel, destroy() cancelling once, a silent unmount) follow the spec as written.
The only change since the last approved version is Drawer importing its hooks from their own modules. That keeps the gesture host out of Drawer's imports and leaves its behaviour unchanged. Nothing further is needed from you.
[Automated review]
|
/approve-spec 32843e1 |
32843e1 to
24c5d6f
Compare
|
/approve-spec 24c5d6f |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. createPanGesture and usePanGesture match the public gesture-hooks spec: each reports a drag as {state, translation, velocity} from a finger, a mouse or a pen, with the 10 px claim, touch-action by axis, a zero-velocity cancel, and a docs page and playing-card example for each layer.
Next step: mark the pull request ready for review, and a maintainer adds the ci:run label so CI runs, since the branch lives in a fork.
- Miss: non-blocking,
packages/core/src/gestures/README.mdline 5 still says nothing in that folder is exported, butcreatePanGesture.tsnow is.
[Automated review]
d68f836 to
67ecd3e
Compare
67ecd3e to
386ad2a
Compare
There was a problem hiding this comment.
Changes requested. The public pan itself matches the public gesture-hooks spec, and its tests pass, but the pull request is now over the size limit for one review.
- At 2,100 authored lines the pull request exceeds the 2,000-line limit, and the new draggable-box commit (two box examples, their docs and test, and the playing-card restyle, 304 lines) is separable from what the spec requires, so move it to its own pull request or rebase #8658 onto this head so that the declared stack
#8655 → #8658holds.
Without that commit the change is the pan with its playing-card examples, and nothing else in it blocks. CI has not run on this head yet, because it is a draft from a fork.
- Non-blocking:
packages/core/src/gestures/README.mdline 5 still says nothing in that folder is exported, butcreatePanGesture.tsnow is.
[Automated review]
…s (spec:AST-107 FR1, FR6)
The public pan: `createPanGesture(element, {axis?, onPan}) → {destroy}` and
`usePanGesture(ref, {axis?, onPan})` over it, reporting
`{state, translation, velocity}` as a drag begins, changes, ends or is
cancelled, under a finger, a mouse and a pen. The translation leaves out
the travel the drag needed to begin, so an element that follows it does
not jump. usePanGesture attaches through useAttachment, Core's one owner
of a node's attachment.
Docs pages for both, with a playing-card example in plain JavaScript and
in React. The card is an image named by its rank and suit, painted in the
theme's surface and text colors. A drag that meets a throw is ignored to
its end, and the next card's face is in place before the thrown card
returns to rest. The core API report, the route lock and the sandbox's routes
list the new exports and pages.
Drawer imports its three hooks from their own modules, as BottomSheet
and Lightbox do, so the hooks entry's new export keeps Drawer off the
gesture host.
386ad2a to
d007fc2
Compare
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #9928.
|
/approve-spec d007fc2 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet.
createPanGesture and usePanGesture match the public gesture-hooks spec: each reports a drag as {state, translation, velocity} from a finger, a mouse or a pen, with the 10 pt claim, touch-action by axis, a zero-velocity cancel, a silent unmount for the hook, and a docs page and playing-card example for each API. The unit, template and browser tests pass on this head and when merged with main.
Next step: since the branch lives in a fork, a maintainer adds the ci:run label so CI runs, once the pull request is out of draft.
- Miss (non-blocking): the Discard button in both playing-card blocks renders as bare text under the theme's reset (no border, background or padding), so the drag's keyboard path does not read as a control (
PanPlayingCard.tsxline 126,PanPlayingCardVanilla.tsxline 153); Core'sButtonor a visible style would fix it.
[Automated review]
| </div> | ||
| </div> | ||
| {/* The drag's keyboard and assistive-technology path (WCAG 2.5.7). */} | ||
| <button type="button" onClick={() => discard(1)}> |
There was a problem hiding this comment.
Non-blocking: under the theme's reset this button renders as bare text (no border, background or padding), so the drag's keyboard path does not read as a control. Core's Button, or a visible style, would show it; PanPlayingCardVanilla.tsx line 153 has the same button.
|
Fixed in #9928, which lands right after this pull request: its Discard button is Core's |
There was a problem hiding this comment.
Changes requested: one public claim needs qualifying. The public pan itself matches the public gesture-hooks spec, and its tests pass on this head with main merged.
- Miss: the changeset (line 6) and both docs pages (
usePanGesture.doc.mjslines 58 and 117,createPanGesture.doc.mjslines 61 and 109) say a finger, a mouse and a pen all drive the pan, but a mouse or pen drag that starts on selectable text inside the element selects that text and reports nothing, so say so and point readers touser-select: none, as both playing-card blocks use it.
Non-blocking: the Discard button in both blocks still renders as bare text, which the switch to Core's Button in #9928 addresses there. CI has not run on this head yet; on this fork branch it needs a maintainer's ci:run label once the draft is ready for review.
[Automated review]
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [feature] `createPanGesture(element, {axis?, onPan})` and `usePanGesture(ref, {axis?, onPan})` report a drag on an element as values: `onPan` gets `{state, translation, velocity}` as the drag begins, changes, ends at the release, or is cancelled, and a finger, a mouse and a pen all drive it. `axis` limits the drag to `'x'` or `'y'`; a drag along the other axis is not reported. `translation` counts from where the drag began, so an element that follows it does not jump, and `velocity` at `ended` is the release velocity in pixels per second. The drag claims its touch at UIKit's 10 pt and leaves presses and long presses alone. `destroy()` removes every listener; the hook creates one pan for the element behind `ref` and destroys it at unmount. |
There was a problem hiding this comment.
"a finger, a mouse and a pen all drive it" is broader than the behavior: a mouse or pen drag that starts on selectable text inside the element is the browser's text selection, and onPan hears nothing. Suggested addition: "A mouse or pen drag that starts on selectable text selects it instead; give text the drag should move user-select: none."
There was a problem hiding this comment.
Done in 14b0fe7: the changeset now carries your sentence. In Chromium and WebKit, a mouse drag that starts on the text selects it and the pan reports nothing; with user-select: none the same drag pans.
| ], | ||
| usage: { | ||
| description: | ||
| 'Reports a drag on an element as values, for a surface the product moves itself: a card that follows the finger, a drawer pulled by its handle. A finger, a mouse and a pen all drive it. The drag claims its touch at UIKit’s 10 pt, as `UIPanGestureRecognizer` does, so a tap stays a tap, and a press or a long press is never reported as a pan. With `axis`, a scroll across that axis that starts on the element stays the page’s; without it, the pan takes a drag in any direction, so a finger’s scroll that starts on the element moves the element instead of the page. A trackpad scroll still scrolls the page. The hook is a thin layer over `createPanGesture`, the same API for code without React. A drag has no keyboard or assistive-technology path of its own: whatever the drag does needs another way to do it, such as buttons or arrow keys (WCAG 2.5.7). The pan moves nothing itself, so Reduce Motion applies to the motion you drive from it: honor the user’s reduced-motion setting in the animation that moves the element. Where a component already offers the behavior, use its prop instead: BottomSheet’s `snapPoints`, Carousel’s `hasSnap`.', |
There was a problem hiding this comment.
Same claim as the changeset: a mouse or pen drag that starts on the element's selectable text selects it and reports nothing (a card with a title dragged by its title does not move). Please say so here and in docsDense (line 117), and name user-select: none as the fix, which both playing-card blocks already apply.
There was a problem hiding this comment.
Done in 14b0fe7, in the usage text and in docsDense: a mouse or pen drag that starts on selectable text selects it, and user-select: none is the fix.
| ], | ||
| usage: { | ||
| description: | ||
| "Reports a drag on an element as values, without React, for a surface the product moves itself: a card that follows the finger, a drawer pulled by its handle. A finger, a mouse and a pen all drive it. The drag claims its touch at UIKit’s 10 pt, as `UIPanGestureRecognizer` does, so a tap stays a tap and a press or a long press is never reported as a pan. Use the `usePanGesture` hook in React. Where a component already offers the behavior, use its prop instead: BottomSheet's `snapPoints`, Carousel's `hasSnap`.", |
There was a problem hiding this comment.
As on the hook's page: a mouse or pen drag that starts on selectable text in the element selects it and the pan reports nothing. Please add that here and in docsDense (line 109).
There was a problem hiding this comment.
Done in 14b0fe7, in the usage text and in docsDense.
…s it The pan's pages and its changeset said a finger, a mouse and a pen all drive it. A mouse or pen drag that starts on selectable text in the element is the browser's text selection instead, and the pan reports nothing; with user-select: none on that text, the same drag pans. Both pages and the changeset now say so and name user-select: none.
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet.
createPanGesture and usePanGesture match the public gesture-hooks spec, and the changeset and both docs pages now say that a mouse or pen drag that starts on selectable text selects it, with user-select: none as the fix. The unit and template tests pass on this head.
Next step: mark the pull request ready for review; since the branch lives in a fork, a maintainer then adds the ci:run label so CI runs.
Non-blocking: the Discard button in both playing-card blocks still renders as bare text; #9928 switches it to Core's Button.
[Automated review]
Stack: #8655 → #9928
The feature, main merged in, and one docs fix.
User need
A product that wants a card to follow a finger has no public way to read a drag from Astryx. It writes its own pointer handling, without Core's rules: claiming at 10 pt, leaving taps and scrolls alone, the release velocity.
Problem and solution fit
createPanGesturereports a drag on an element as values, andusePanGestureis its React layer. Both reuse Core's own pan, so finger, mouse and pen follow Core's rules. Each page shows a playing card that follows the finger.Current specification
spec:AST-107FR1 (a drag reported as{state, translation, velocity}) and FR6 (one API at two layers); FR4 keeps recognizers internal.Public delta and caller burden
New exports:
createPanGesture(element, {axis?, onPan})returning{destroy},usePanGesture(ref, {axis?, onPan}), and the typesPanGesture,PanGestureEvent,PanGestureOptionsandPanGestureState.A caller decides one thing: whether to limit the drag to an axis. With
axis, a scroll across it stays the page's. Without it, a finger's scroll on the element moves it; a trackpad scroll still scrolls the page. A drag has no keyboard path, so the cards add a Discard button (WCAG 2.5.7).Design and alternatives
The factory runs a
PanGestureRecognizeron the document's gesture host. The hook attaches it through Core's attachment owner (useAttachment) and never callsonPanfrom an effect (architecture:react-component-runtimeINV2). Exporting the recognizer is ruled out by FR4; a hook alone would leave code without React out (FR6). Drawer imports each hook from its own module: the hooks entry's new export put the gesture host in Drawer's static graph.Evidence
{0, 0}.destroymid-drag reportscancelledonce, the hook's unmount reports nothing, and import does no work.Scope
Testing
Local gates at 14b0fe7: the feature (d007fc2), main 8fdf6be merged in (f6c6922), and the docs fix on top.
user-select: noneon the text, the drag on it pans in both engines. This was a local probe, not part of the diff; the changeset and both pages now say so.pnpm build: the CLI's node tests with the route manifest and the touch-support bundle, 570 files and 9,894 tests passed. The pan's unit tests passed, 14 of 14.routes.lock.jsonmatches the generator.check:knowledge,check:changesets, Prettier and strict ESLint: clean.node scripts/pre-review.mjs --base 8fdf6be377: no findings, 1,819 authored lines.Earlier evidence, at the feature commit:
Local gates at 24c5d6f, one commit on main 859df5f. This head adds one README line: the gestures folder's README now says the package exports
createPanGestureand its types from there. It is a pure rebase of 32843e1:git patch-id --stablereads e0aa09910fc90c40 for both, andgit range-diffmarks the commit equal.tsc --project packages/core/tsconfig.json --noEmit, strict ESLint,pnpm lintand Prettier on the changed files,check:*andpnpm review:pre(no findings): clean.useScrollLockImports' Drawer case fails without the Drawer change. The pan blocks, the touch-support bundle and the doc compiler's inputs: 33 passed.CI=1,LP_NUM_THREADS=4, one worker), ten runs: 670 passed and 120 skipped (the Chromium-only cases), none failed.