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
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: against main this pull request carries 23,228 authored changed lines, and all but its own 67-line useMenuPress commit come from the dependency stack it names (#8076, #8298, #8446, #8078), so it cannot be reviewed or merged as the one change it proposes. Please land those pull requests first, then merge main, so the diff here is only the useMenuPress change. That change itself follows spec:AST-094 FR25 and FR29: with longPressDelayMs unset, a finger opens the menu on press under a coarse primary pointer, and longPressDelayMs: 500 keeps the released hold.
[Automated review]
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
There was a problem hiding this comment.
Changes requested: against main this pull request carries 22,561 authored changed lines, and all but its own 67-line useMenuPress commit come from the dependency stack it names (#8076, #8298, #8446, #8078), so it cannot be reviewed or merged as the one change it proposes. Please land those pull requests first and then merge main, so the diff here is only the useMenuPress change, which itself follows spec:AST-094 FR25 and FR29. In that commit, please also bring the hook's other descriptions up to date: the file header in useMenuPress.ts and the triggerProps, onTriggerPress and usage text in useMenuPress.doc.mjs still describe only a finger held for the delay, while with longPressDelayMs unset a finger on a touch screen now opens the menu as it presses.
[Automated review]
| * - A mouse press on the trigger opens the menu at once. A finger held for | ||
| * `longPressDelayMs` opens it and the opening press follows the settle | ||
| * rule (see menuPressGesture.ts). DropdownMenu's own entry | ||
| * (`useDropdownMenuPress`, touch model `'uikit'`, `spec:AST-094` FR29) | ||
| * instead opens it as a finger presses on a trigger in a fixed bar, and | ||
| * inside a scroll view once the 150 ms content-touch delay has passed with | ||
| * no scroll; the opening finger then travels UIMenu's handoff distance | ||
| * before it picks. |
There was a problem hiding this comment.
This header now misdescribes the public hook: with this commit, useMenuPress with longPressDelayMs unset takes the same 'uikit' path as useDropdownMenuPress where the primary pointer is coarse (lines 402-409), so a finger opens the menu as it presses and picks only after the handoff. Worth saying so here, and in the onTriggerPress JSDoc (line 201) and the triggerProps comment (line 274).
| description: | ||
| 'How long a finger must rest on the trigger before the menu opens under it.', | ||
| default: '500', | ||
| "How long a finger must rest on the trigger before the menu opens under it. Unset, a finger on a touch screen (a coarse primary pointer) opens the menu as it presses, or after 150 ms inside a scroll view, and a fine primary pointer waits 500 ms. Set, it is that wait everywhere. A tap released sooner opens the menu through the trigger's click.", |
There was a problem hiding this comment.
This entry now states the press-open default, but options.onTriggerPress (line 52), triggerProps (line 102), the usage text (line 119) and the dense entries (lines 157, 164, 179, 186) still describe only a finger held for the delay. One clause in each, such as "with longPressDelayMs unset, a finger on a touch screen opens the menu as it presses (after 150 ms in a scroll view)", keeps the consumer docs consistent.
There was a problem hiding this comment.
Changes requested: against main this pull request carries 22,094 authored changed lines, and all but its own 67-line useMenuPress change come from the dependency stack it names (#8076, #8298, #8446, #8078), so it cannot be reviewed or merged as the one change it proposes. Please land those pull requests first and then merge main, so the diff here is only the useMenuPress change, which itself follows spec:AST-094 FR25 and FR29. In that change, please also bring the hook's other descriptions up to date: the options.onTriggerPress, triggerProps and usage text in useMenuPress.doc.mjs, and the matching JSDoc and file header in useMenuPress.ts, still describe only a finger held for the delay, while with longPressDelayMs unset a finger on a touch screen now opens the menu as it presses.
[Automated review]
|
Done at a8ca433. This pull request's own change is vjeux/astryx@8014a27...a8ca433.
|
There was a problem hiding this comment.
Changes requested: against main this pull request carries 22,269 authored changed lines, and all but its own 110-line useMenuPress change come from the open dependency stack (#8076, #8298, #8446, #8078), so it cannot be reviewed or merged as the one change it proposes. Please land those pull requests first, together with #8627, which now carries the opening finger's handoff that #8078 no longer includes, and then merge main, so the diff here is only the useMenuPress change. That change itself follows spec:AST-094 FR25 and FR29, and the hook's descriptions now match it.
[Automated review]
There was a problem hiding this comment.
Changes requested: against main this pull request carries 19,080 authored changed lines, and all but its own 113-line useMenuPress change come from the open dependency stack (#8446, #8076, #8298, #8627, #8078), so it cannot be reviewed or merged as the one change it proposes. Please land those pull requests first and then merge main, so the diff here is only the useMenuPress change, which itself follows spec:AST-094 FR25 and FR29. In that change, please also name the handoff in .changeset/menu-press-public-default.md: with longPressDelayMs unset on a touch screen, the opening finger now picks only after moving 10 px, longPressDelayMs: 500 keeps the released behavior, and the carried menu-press-handoff changeset currently says the public hook is unchanged.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feature] `useMenuPress` with no `longPressDelayMs` now opens the menu as a finger presses on a touch screen too (a coarse primary pointer), with DropdownMenu's timing (`spec:AST-094` FR25, FR29); pass `longPressDelayMs: 500` to keep the previous 500 ms hold. Where the primary pointer is fine it keeps the 500 ms hold. |
There was a problem hiding this comment.
This note leaves out the handoff the hook now gets: with longPressDelayMs unset under a coarse pointer, the opening finger picks only after moving 10 px instead of following the settle rule (acting once in the menu or 300 ms after the open), and longPressDelayMs: 500 keeps both the hold and the settle rule. The carried menu-press-handoff.md still says "the public useMenuPress hook [is] unchanged"; if it is unreleased when this merges, that clause should go too (spec:AST-017 FR3).
There was a problem hiding this comment.
Changes requested: against main this pull request carries 18,655 authored changed lines, and all but its own 113-line useMenuPress change come from the open dependency stack (#8446, #8076, #8298, #8627, #8078), so it cannot be reviewed or merged as the one change it proposes. Please land those pull requests first and then merge main, so the diff here is only the useMenuPress change, which itself follows spec:AST-094 FR25 and FR29. In that change, please also name the handoff in .changeset/menu-press-public-default.md: with longPressDelayMs unset on a touch screen, the opening finger now picks only after moving 10 px, longPressDelayMs: 500 keeps the released behavior, and the carried menu-press-handoff changeset still says the public hook is unchanged.
[Automated review]
There was a problem hiding this comment.
Changes requested: against main this pull request carries 18,827 authored changed lines, and all but its own 113-line useMenuPress change come from the open dependency stack (#8446, #8076, #8298, #8627, #8078), so it cannot be reviewed or merged as the one change it proposes. Please land those pull requests first and then merge main, so the diff here is only the useMenuPress change, which itself follows spec:AST-094 FR25 and FR29. In that change, please also name the handoff in .changeset/menu-press-public-default.md: with longPressDelayMs unset on a touch screen, the opening finger now picks only after moving 10 px, longPressDelayMs: 500 keeps the released behavior, and the carried menu-press-handoff changeset still says the public hook is unchanged.
[Automated review]
There was a problem hiding this comment.
Changes requested: against main this pull request carries 30,414 authored changed lines, and all but its own 154-line useMenuPress change come from the open dependency stack (#8446, #8076, #8360, #8298, #8627, #8078), so it cannot be reviewed or merged as the one change it proposes. Please land those pull requests first and then merge main, so the diff here is only the useMenuPress change, which itself follows spec:AST-094 FR25, FR29 and FR43. In that change, please drop "and a fine primary pointer waits 500 ms" from the longPressDelayMs description in useMenuPress.doc.mjs, since with no delay set nothing waits 500 ms on any device, and update the title and description, which still say a laptop's touchscreen keeps the old wait.
[Automated review]
| description: | ||
| 'How long a finger must rest on the trigger before the menu opens under it.', | ||
| default: '500', | ||
| "How long a finger must rest on the trigger before the menu opens under it. Unset, a finger on every device opens the menu as it presses, or after 150 ms inside a scroll view, and a fine primary pointer waits 500 ms. Set, it is that wait everywhere. A tap released sooner opens the menu through the trigger's click.", |
There was a problem hiding this comment.
With longPressDelayMs unset the hook always passes 'uikit' (useMenuPress.ts:360-361), so nothing waits 500 ms here: a finger or pen opens on the press, or after 150 ms in a scroll view (menuTriggerPress.ts:193-197, :225-241), and a mouse opens at once (menuPressGesture.ts:241-248). The clause "and a fine primary pointer waits 500 ms" contradicts the first half of this sentence, the JSDoc (useMenuPress.ts:190-194) and module:DropdownMenu/useMenuPress's long-press delay row; dropping it keeps the docs true.
There was a problem hiding this comment.
Changes requested: against main this pull request still carries 30,414 authored changed lines, and all but its own 154-line useMenuPress change come from the open dependency stack (#8446, #8076, #8360, #8298, #8627, #8078), so it cannot be reviewed or merged as the one change it proposes. Please land those pull requests first and then merge main, so the diff here is only the useMenuPress change, which follows spec:AST-094 FR25, FR29 and FR43 and whose option documentation now matches the code. When you do, please also update the title and description, which still say the new default applies only where the primary pointer is coarse and leave #8360 out of the merge order.
[Automated review]
vjeux
changed the title
… moves (spec:AST-094 FR29, FR43) Under a finger or pen, on every device, a DropdownMenu or MoreMenu opened under a resting finger picked the row beneath it once the press had lasted 300 ms. The press machine's open phase now takes UIMenu's handoff: until the finger is 10 px, in a straight line, from the first position the open menu sees, nothing lights and a lift acts on nothing; then it slides and picks as usual. A mouse keeps the settle rule, and presentation="popover" pins the pointer model. - menuPressGesture: the handoff state and transitions, on internal types; the released menuPressStep keeps its public shapes. - useMenuPress: the internal useDropdownMenuPress entry takes a touch model; the public hook is unchanged. - menuPressValues: UIKit's handoff distance, copied from the constants table so a menu bundles the number, not the table. - DropdownMenu and MoreMenu run the touch model unless presentation is "popover"; docs and a changeset say so. - A real-browser spec (DropdownMenuTouchHandoff); core declares the interaction harness it imports.
c9e4fc2 to
5d59d96
Compare
github-actions
Bot
removed
the
needs:design-review
There was a problem hiding this comment.
Changes requested. The useMenuPress change itself follows spec:AST-094 FR25, FR29 and FR43, and its tests pin both the new unset default and the longPressDelayMs: 500 way back.
- Against
mainthis pull request carries 2,767 authored lines, 2,622 of them from the open pull requests #8627 and #8078, so please land those first and mergemain, leaving this diff as only the 145-lineuseMenuPresschange. isInScrollViewinpackages/core/src/hooks/menuTriggerPress.tscalls the shared native-scroll probe and then walks the ancestors again with its own copy of the scrollable-container and fixed-ancestor rule, so please returncreateNativeScrollProbe(control).isInsideScrolleralone and extend the probe if a case is missing (this code comes from #8078, so the fix can land there).
Two small comment cleanups are suggested inline (non-blocking).
[Automated review]
| const doc = control.ownerDocument; | ||
| const view = doc.defaultView; | ||
| const root = doc.scrollingElement ?? doc.documentElement; | ||
| for ( | ||
| let node = control.parentElement; | ||
| node != null && node !== root && node !== doc.body; | ||
| node = node.parentElement | ||
| ) { | ||
| const style = view?.getComputedStyle(node); | ||
| const scrolls = | ||
| style != null && | ||
| ((/(auto|scroll)/.test(style.overflowY) && | ||
| node.scrollHeight > node.clientHeight) || | ||
| (/(auto|scroll)/.test(style.overflowX) && | ||
| node.scrollWidth > node.clientWidth)); | ||
| if (scrolls) { | ||
| return true; | ||
| } | ||
| if (style?.position === 'fixed') { | ||
| return false; | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
This walk re-implements the rule that line 117 already gets from the shared probe. createNativeScrollProbe(control).isInsideScroller counts a scroller with room unless it lies beyond a position: fixed ancestor (gestures/nativeScroll.ts:69-74, :137-139), and PressTracker's 150 ms row delay uses that answer alone. With the function reduced to return createNativeScrollProbe(control).isInsideScroller;, every useMenuPress, DropdownMenu and MoreMenu test still passes, the fixed-bar and sticky cases included. Keeping one definition means a fix to the probe also reaches the menu trigger.
| // A trigger in a fixed bar opens as the finger presses, and the page | ||
| // holds still under it as a bar's does in UIKit. The finger's compatibility mouse events must not follow it | ||
| // onto the trigger: their mousedown would take focus back from the | ||
| // menu. |
There was a problem hiding this comment.
Non-blocking: the first sentence restates the branch, and line 230 runs to 116 characters. Suggest keeping only the reason:
// Cancel the compatibility mouse events. Their mousedown would take focus
// back from the menu.| // because the pointer handlers below are stable and the hook is created | ||
| // after `fireLongPress`. | ||
| const longPressApiRef = useRef<LongPressDetectorHandlers | null>(null); | ||
| // Which detector the live press uses: inside a scrolling region or not. |
There was a problem hiding this comment.
Non-blocking: this restates the 'plain' | 'scroller' type on the next line. Consider dropping it.
… carry positions The released menuPressStep hands its events to the internal transition, which starts the finger's handoff when a move carries x and y. The public gesture drops the handoff origin, so a caller whose events carried positions got no pick from the opening finger. menuPressStep now strips the positions first, and a test whose events carry them pins it. Three comments are reworded.
5d59d96 to
cdad9b0
Compare
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested. The useMenuPress change itself follows spec:AST-094 FR25, FR29 and FR43, and its tests pin both the new unset default and the longPressDelayMs: 500 way back.
- Against
mainthis pull request carries 2,851 authored lines, 2,718 of them the change of the open pull requests #8627 and #8078, so please land those first and mergemain, leaving this diff as only the 145-lineuseMenuPresschange. - The JSDoc on the exported
useMenuPressinpackages/core/src/hooks/useMenuPress.tsstill says "A finger held on the trigger opens the menu after the long-press delay" and that the settle rule decides the lift, so please say that with nolongPressDelayMsa finger opens the menu as it presses and picks after the 10 px handoff.
The earlier duplicated scroll-view walk in isInScrollView is fixed. A few small wording cleanups are suggested inline (non-blocking).
[Automated review]
| expect(trigger).toHaveAttribute('aria-expanded', 'true'); | ||
| }); | ||
|
|
||
| it('the pointer model (the public hook\u2019s) keeps the settle rule: once the press has settled, a still finger\u2019s lift over a row acts', () => { |
There was a problem hiding this comment.
Non-blocking: since this change the public hook with no delay takes the touch model, so "the public hook’s" no longer names the pointer model. Suggest "the pointer model (a set delay’s)", and the same fix for the harness comment at line 118 ("without touchModel it behaves as the public hook").
| // tolerance and the timer are what matter, and they have one home. Inside | ||
| // a scroll view the touch model waits for the content-touch delay | ||
| // (`scroll.delaysContentTouchesMs`), so a scroll can claim the touch first. | ||
| // The press picks its detector when it lands. | ||
| // With `touchModel: 'uikit'`, only a trigger inside a scroll view waits: | ||
| // the content-touch delay, within the drift allowance. One in a fixed bar | ||
| // opens as the finger presses, so its detector never runs. |
There was a problem hiding this comment.
Non-blocking: this says the scroll-view wait twice. Suggest keeping only:
// With `touchModel: 'uikit'`, only a trigger inside a scroll view waits for
// the content-touch delay, within the drift allowance. One in a fixed bar
// opens as the finger presses, so its detector never runs.…resses (AST-094 FR29) Under a finger or pen, on every device, a DropdownMenu or MoreMenu trigger in a fixed bar opens the menu as the finger presses, as UIMenu's pull-down button does. Inside a scroll view it opens once the 150 ms content-touch delay has passed with no scroll, within a 13 px drift allowance. The opening finger then picks only after the handoff distance. A mouse is unchanged, and presentation="popover" keeps the pointer model. - menuTriggerPress.ts: the trigger half of the press model (the press-down open, the held finger, the scroll-view wait), reached only by menus with a trigger. - useMenuPress: the trigger-less callers inside core's own menus (sub-menus, ContextMenu, Selector's list and the sheet's action list) take the internal useMenuSurfacePress, so they do not bundle the trigger half. - menuPressValues: UIKit's hold-open slop and content-touch delay, copied from the tables with drift tests. - A real-browser spec (DropdownMenuTouchPress) and an accessibility case. - The touch-support bundle check names the eight imports that now carry the trigger half's scroll-view wait.
cdad9b0 to
26e5a0b
Compare
There was a problem hiding this comment.
Changes requested. The useMenuPress change itself follows spec:AST-094 FR25, FR29 and FR43, its unset and longPressDelayMs: 500 paths are tested, and the hook's JSDoc now describes the new default.
- Against
mainthis pull request carries 2,878 authored lines, 2,709 of them the change of the open pull requests #8627 and #8078, so please land those first and mergemain, leaving this diff as only the 169-lineuseMenuPresschange. - No test drives the public hook in a scroll view: deleting the
isInScrollViewoption thatuseMenuPressforwards inpackages/core/src/hooks/useMenuPress.tsstill passes every test while the hook then opens on the press instead of after the 150 ms content-touch delay, so please add a public-hook case inside the test file'sScrollViewhelper that is closed at 149 ms and open at 150 ms.
Non-blocking: the comments in menuTriggerPress.ts and menuScrollView.ts and the isInScrollView JSDoc still say only DropdownMenu passes the scroll probe, which the public hook now does too.
[Automated review]
| clientY: 0, | ||
| }); | ||
| expect(onOpenChange).toHaveBeenCalledWith(true); | ||
| }); |
There was a problem hiding this comment.
These public-hook cases cover a trigger in no scroll view only. The hook's 150 ms scroll-view wait comes from passing isInScrollView in useMenuPress.ts; deleting that line still passes every test in this file, and the hook then opens on the press inside a scroll view. A case like the one at line 605, but rendering PublicHarness inside ScrollView (closed at 149 ms, open at 150 ms), would pin it.
| * not bundle the long-press detectors. Only DropdownMenu passes the scroll | ||
| * probe, which only the touch model reads. |
There was a problem hiding this comment.
Non-blocking: since this change the public useMenuPress passes the scroll probe too, so this is no longer true (the same sentence is in menuScrollView.ts and the isInScrollView JSDoc in useMenuPress.ts). Suggest: "DropdownMenu and the public useMenuPress pass the scroll probe. Only the touch model reads it."
…ry device (AST-094 FR25) With `longPressDelayMs` unset, the public useMenuPress now follows DropdownMenu's finger rules on every device: a finger opens the menu as it presses on a trigger in a fixed bar, and inside a scroll view once the 150 ms content-touch delay has passed with no scroll; the opening finger then picks a row only after the handoff. A delay that is set keeps the released wait everywhere. A mouse is unchanged. - useMenuPress: an unset delay selects the UIMenu touch model; its option and trigger docs, the hook doc and the API report drop the 500 ms default. - meta#8627's changeset no longer says the public hook is unchanged. - Tests: the trigger cases with no delay set, on a fine primary pointer too. - A [feature] changeset.
26e5a0b to
753cecf
Compare
There was a problem hiding this comment.
Changes requested. The useMenuPress change itself follows spec:AST-094 FR25, FR29 and FR43, and its unset, scroll-view and longPressDelayMs: 500 paths are now tested.
- Against
mainthis pull request carries 2,905 authored lines, 2,697 of them the change of the open pull requests #8627 and #8078, so please land those first and mergemain, leaving this diff as only the 208-lineuseMenuPresschange.
[Automated review]
Re-applied onto current main, stacked on #8078. Comments drop record ids for the source-comment rule.
User need
An app that builds its own menu on the public
useMenuPresshook still makes a finger wait 500 ms to open it, while DropdownMenu opens as the finger presses. Two menus in one app then feel different under the same finger.What led to it:
spec:AST-094FR25 says an unset delay follows the platform's rule (FR29), while a delay the caller sets is kept everywhere; a reviewer asked to split this out of #8078.Problem and solution fit
When
longPressDelayMsis not set, the hook now runs DropdownMenu's touch model for a finger on every device (spec:AST-094FR43): the menu opens as the finger presses (in a scroll view, after the 150 ms content-touch delay), and the opening finger picks only after moving 10 px. A set delay keeps its value, and a mouse is unchanged. The hook now passes the touch model's scroll probe, so the touch-support bundle check names the nav, breadcrumb and tab menus that take it.Current specification
spec:AST-094FR25, FR29 and FR43 (current);module:DropdownMenu/useMenuPress(current). No record changes.Public delta and caller burden
No new option or type. A caller that sets no
longPressDelayMsgets the new behavior on every device;longPressDelayMs: 500keeps the previous hold. The API report drops the option's= 500default. The changeset names the 10 px handoff and that way back, and #8627's changeset, stacked for one release with this one, no longer says the public hook is unchanged.Design and alternatives
Scope
Testing
pnpm vitest run packages/core/src/hooks/useMenuPress.test.tsx:useMenuPress.tsrestored.touchModelan untyped caller passes still reaches nothing.The
longPressDelayMsdescription inuseMenuPress.doc.mjsno longer says a fine primary pointer waits 500 ms with no delay set.Local gates at 753cecf: the menu and nav units pass (1,918 of 1,918), the touch-bundle test (8 of 8) and the core type check; with main's default 2 of the hook's cases fail, and without the hook's scroll-probe forward its scroll-view case fails. The full gates are running.
Stack: #8627 → #8078 → this PR. Review only this PR's change: vjeux/astryx@4ad7e96...753cecf