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:spec-owner-review
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 needs an engineering owner's decision before it can be approved: the touch motion follows the draft AST-094, whose FR4 narrows the current theme-tokens architecture (INV3) to pointer presentations, and the useMenuPress FR8, FR9, DEC-4 and new DEC-8 are still proposals. 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. Whatever the ruling, these changes are still needed:
- Rebase onto the landed predecessors, or split, so this pull request carries its own change rather than 26,186 changed lines.
- Keep the pan-outside dismissal's pointer set across renders in
DropdownMenu.tsx, becausecloseMenuchanges identity on every render and the set starts empty again. - Bring
useMenuPress.spec.md(the 500 ms row, FR9 and DEC-4) in line with the 150 ms hold and the 10 px handoff, and name the oldlongPressDelayMsdefault and the option that restores it in the changeset.
[Automated review]
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
f4634c0 to
9f00657
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: the touch motion follows the draft AST-094, whose FR4 narrows the current theme-tokens architecture (INV3) to pointer presentations, and the useMenuPress FR8, FR9, DEC-4 and DEC-8 are still proposals. 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. Whatever the ruling, these changes are still needed:
- Rebase onto the landed predecessors, or split, so this pull request carries its own change rather than 28,469 changed lines.
- Show the menu's closing copy in the top layer instead of beside the popover with
position: fixedandz-index: 2147483647, which AST-027 DEC-3 rules out for menus. - Keep the pan-outside dismissal's pointer set across renders in
DropdownMenu.tsx. - Bring
useMenuPress.spec.md(the 500 ms row, FR9 and DEC-4) in line with the 150 ms hold and the 10 px handoff. - Name the old
longPressDelayMsdefault and its restoring option in the changeset, and recommend onlytouch-action: pinch-zoomfor a press-open trigger.
[Automated review]
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
|
Agreed: one implementation.
|
9f00657 to
e88dc4c
Compare
github-actions
Bot
added
the
needs:design-review
|
/approve-spec b08ab80 |
There was a problem hiding this comment.
Approval waits on an engineering owner. This head rewords FR31 in the current AST-094 record ("until then the open fades and closes with no shrink"), and the earlier /approve-spec covered the previous head, so FR31's added sentences and the sentences this pull request adds to the DropdownMenu record's FR21 need an engineering owner to comment /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner). The reworded FR31 fixes its earlier mismatch with FR21, and the scope now names useMenuPress. These changes are still required:
useMenuSurfaceMotion.tsandmenuSurfaceMotion.tsstill copy Popover's surface motion (boxOf, the presentation hold,motionFor,closeStart, the enter and the exit), so the menu and Popover need one surface motion that both call.- Miss: the grow is measured from
buttonRef.current(DropdownMenu.tsx:1157-1164), the Button that a finger's press scales to 0.98, while the layer-runtime record's INV22 has a layer anchor to the trigger's untransformed box, so the grow should measure a box the press does not scale. - At 6,667 authored lines, the
Stack:line does not hold, because the heads of #8485 and #8508 are not contained in this head, so restack once #9764, #9769 and #8078 land or make the line hold for every pull request it lists. - AST-094's platform evidence needs Android emulator runs at this head (animations at normal scale and removed, plus a slow-device profile) and the iPhone runs taken again at this head, since this range's own changes since the earlier device runs stop them from carrying.
There was a problem hiding this comment.
Changes requested. An engineering owner's /approve-spec for this head settles the sentences this pull request adds to the AST-094 FR31 and DropdownMenu FR21 records, so the spec side is done. These changes are still required:
useMenuSurfaceMotion.tsandmenuSurfaceMotion.tsstill copy Popover's surface motion (boxOf, the presentation hold,motionFor,closeStart, the enter and the exit), so the menu and Popover need one surface motion that both call.- Miss:
loadMotion(useMenuSurfaceMotion.ts:63-75) re-implementscreateModuleLoaderfromutils/moduleLoader.ts, whichlazyMenuBottomSheet.tsin the same folder already calls and the component import boundaries record (INV1) now requires, so load the grow's module through it. - Newly live check: the grow's code starts loading at the page's first touch (
useMenuSurfaceMotion.ts:102-116), a moment no record names as the component import boundaries record's INV2 requires, and a failed load gives no development warning (INV6), so start the load at the trigger's press or have FR21 name that moment, and warn once in development. - Miss: the grow is measured from
buttonRef.current(DropdownMenu.tsx:1157-1164), the Button that a finger's press scales to 0.98, while the layer-runtime record's INV22 has a layer anchor to the trigger's untransformed box, so the grow should measure a box the press does not scale. - At 6,667 authored lines, the
Stack:line does not hold, because the heads of #8485 and #8508 are not contained in this head, so restack once #9764, #9769 and #8078 land or make the line hold for every pull request it lists. - AST-094's platform evidence needs Android emulator runs at this head (animations at normal scale and removed, plus a slow-device profile) and the iPhone runs taken again at this head, since this range's own changes since the earlier device runs stop them from carrying.
The grow's code now loads through the shared module loader that the menu's bottom sheet uses, as the import boundaries require. A failed load warns once in development builds, and the next open still asks again. The DropdownMenu record's FR21 names when the code loads: at a trigger's first touch, or the page's first touch where touch is the primary input.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: the sentence this head adds to the DropdownMenu record's FR21 (when the grow's code loads), like the 250 ms fallback beside it, rests on DropdownMenu loading its grow with import(), which the package stylesheets record's DEC-5 and the React component runtime record's INV13 currently rule out for a component's own touch code. An engineering owner either keeps the lazy grow (commenting /approve-spec <full head SHA> on the pull request and amending DEC-5 and INV13) or has it load statically, and the author may record that decision as an engineering owner. Required beside that decision:
- Miss:
useMenuSurfaceMotion.ts:42-55loads the menu's own grow code and the rect morph withimport(), which AST-080 DEC-5 rejects ("a component does not lazy-load its own"), so load it statically and justify the cost, or have DEC-5 and INV13 amended first. useMenuSurfaceMotion.tsandmenuSurfaceMotion.tsstill copy Popover's surface motion (boxOf, the presentation hold,motionFor,closeStart, an exit-copy module each), so the menu and Popover need one surface motion that both call.- Miss: the grow is measured from
buttonRef.current(DropdownMenu.tsx:1157-1164), the Button that a finger's press scales to 0.98, while the layer-runtime record's INV22 has a layer anchor to the trigger's untransformed box, so the grow should measure a box the press does not scale. - Miss:
.changeset/motion-rect-morph.mdand.changeset/motion-morph-path.mdsay no component uses the rect morph yet, while DropdownMenu's grow plays on it at this head, so drop that clause or land #9764 and #9769 first. - At 6,743 authored lines, the
Stack:line does not hold, because the heads of #8485 and #8508 are not contained in this head, so restack once #9764, #9769 and #8078 land or make the line hold for every pull request it lists. - AST-094's platform evidence needs Android emulator runs at this head (animations at normal scale and removed, plus a slow-device profile) and the iPhone runs taken again at this head, since this head's change to
useMenuSurfaceMotion.tsstops the earlier device runs from carrying.
The shared module loader, the development warning and the named load moment fix the three loading findings from the last review.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: the rect morph's paths, the geometry touch presentations share when they grow out of a source rect and shrink back into it. `lerpPath` (a straight line between two rects) and `edgeGrowPath` (UIKit's popover, growing out of a point on its edge) name the rect an element shows at each progress, and say how an interrupted motion continues in their own law; `morphCurves` turns a path into compositor curves (`translate`, `scale` and a companion `opacity`). Not exported from the package entry point; no component uses them yet. |
There was a problem hiding this comment.
Miss: "no component uses them yet" is no longer true in this range: DropdownMenu's grow plays edgeGrowPath (menuSurfaceMotion.ts:85-91). Could this clause go, or #9764 land first?
…and the rect morph use timedCurve easedProgress moves to the progress helpers byte for byte, so Popover and SegmentedControl read the same positions and velocities as on main. timedCurve is the pager's curve as on main, with one change: from rest, its velocity at t <= 0 is the curve's own starting slope. The rect morph plays its timed motion on timedCurve from 0 to 1. callerMotionValues.test.ts pins easedProgress and the pager on a fixed grid, bit for bit, to main's values. Based on vjeux's test (vjeux/astryx coverage/rect-morph-first-instant)
… speed A linear move of 300 px over 0.3 s, retargeted at t = 0, reads 0 px/s on main, so the retarget starts from rest. Now it reads about 1000 px/s, the curve's own starting slope, and the retarget keeps that speed. The position stays continuous. Based on vjeux's test (vjeux/astryx coverage/rect-morph-first-instant)
On main a 0 ms animated scroll reads NaN at t = 0 and its start position before it. timedCurve's 0 ms guard puts it on its target with no speed, as the rect morph's 0 ms timing does.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: the sentences this pull request adds to the touch-presentations record's FR31 and the DropdownMenu record's FR21 (when the grow's code loads, and the fade before it does) have no spec approval at this head, and they rest on DropdownMenu loading its grow with import(), which the package stylesheets record's DEC-5, the React component runtime record's INV13 and the component import boundaries record's INV8 currently rule out for a component's own touch code. An engineering owner either keeps the lazy grow (commenting /approve-spec <full head SHA> on the pull request and amending those three clauses) or has it load statically; the author may record that decision as an engineering owner. Required beside that decision:
- Miss:
useMenuSurfaceMotion.ts:42-56loads the menu's own grow code and the rect morph withimport(), which DEC-5 rejects ("a component does not lazy-load its own"), so load it statically and justify the cost, or have DEC-5, INV13 and INV8 amended first. useMenuSurfaceMotion.tsandmenuSurfaceMotion.tsstill copy Popover's surface motion (boxOf, the presentation hold,motionFor,closeStart, an exit-copy module each), so the menu and Popover need one surface motion that both call.- Miss: the grow is measured from
buttonRef.current(DropdownMenu.tsx:1159-1166), the Button that a finger's press scales to 0.98, while the layer-runtime record's INV22 has a layer use the trigger's untransformed box, so the grow should measure a box the press does not scale. - Miss:
.changeset/motion-rect-morph.mdand.changeset/motion-morph-path.mdsay no component uses the rect morph yet, while DropdownMenu's grow plays on it, so drop that clause or land #9764 and #9769 first. - At 6,737 authored lines the
Stack:line does not hold, because the heads of #8485 and #8508 are not contained in this head, so restack once #9764, #9769 and #8078 land or make the line hold for every pull request it lists. - The touch-presentations record's platform evidence needs Android emulator runs at this head (animations at normal scale and removed, plus a slow-device profile) and the iPhone runs taken again at this head, since this head's merge changes files the pull request changes.
…functions A float64 hash of easedProgress's values is not the same on every platform: macOS arm64 and CI's Linux give different hashes for main's code. The test now keeps copies of easedProgress and solveAnimatedPageScroll as they are on main (ebb9dde), and checks that the shipped functions give the same number, bit for bit (Object.is), for each position and velocity on the grid. From rest, the pager's velocity at t <= 0 stays the one value left out. Based on vjeux's test (vjeux/astryx coverage/rect-morph-first-instant)
|
/approve-spec bdb2003 |
There was a problem hiding this comment.
Changes requested. An engineering owner's /approve-spec for this head settles the text this pull request adds to the touch-presentations record's FR31 and the DropdownMenu record's FR21, the lazy-loaded grow included, so no spec approval is pending. Earlier points on the page-wide touch listener, the shared module loader, the fallback entrance's input hold, the load moment and the useMenuPress scope are fixed at this head. These changes are still required:
- Miss:
useMenuSurfaceMotion.ts:42-56loads the menu's own grow code and the rect morph withimport()while the package stylesheets record's DEC-5 ("a component does not lazy-load its own"), the React component runtime record's INV13 and the component import boundaries record's INV8 still forbid it, so amend those three clauses onmainto match the approved FR21, or load the grow statically. useMenuSurfaceMotion.tsandmenuSurfaceMotion.tsstill copy Popover's surface motion (boxOf, the presentation hold,motionFor,closeStartand an exit-copy module each), so the menu and Popover need one surface motion that both call.- Miss: the grow is measured from
buttonRef.current(DropdownMenu.tsx:1159-1166), the Button that a finger's press scales to 0.98, while the layer-runtime record's INV22 has a layer use the trigger's untransformed box, so the grow should measure a box the press does not scale. - Miss:
.changeset/motion-rect-morph.mdand.changeset/motion-morph-path.mdstill say no component uses the rect morph while DropdownMenu's grow plays on it, so drop that clause or land #9764 and #9769 first. - At 7,010 authored lines the
Stack:line does not hold, because the heads of #8485 and #8508 are not contained in this head, so restack once #9764, #9769 and #8078 land or make the line hold for every pull request it lists. - The touch-presentations record's platform evidence needs Android emulator runs at this head (animations at normal scale and removed, plus a slow-device profile) and the iPhone runs taken again at this head, since this head's merges change
motion/progress.tsandmotion/rectMorph.ts, which the pull request changes.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: the rect morph, one way for a touch presentation to grow out of a source rect and shrink back into it. `createRectMorph` presents an element along a path (`lerpPath`, `edgeGrowPath`) and dismisses it back on a copy or on itself, each continuing the other from the rect on screen. Each play takes the timing the component's motion record names (AST-094 FR45): a duration and easing, or a spring, starting from the velocity it is given. Reduced Motion is left to the animator. `easedProgress` moves to the progress helpers with the same values. `timedCurve` joins them: a timed motion that keeps a retargeted velocity, for the pager and the rect morph. Not exported from the package entry point; no component uses it yet. |
There was a problem hiding this comment.
There was a problem hiding this comment.
Approval waits on an engineering owner. This head adds sentences to the DropdownMenu record's FR21 (a pen's open grows the menu, the grow follows the popover's path, taps stay held until the menu shows) beside the touch-presentations record's FR31, and a bullet to the Popover record naming createRectMorphSurfaceMotion and the surface-motion options DropdownMenu passes; the earlier /approve-spec covered a previous head, so an engineering owner records the decision on this text by commenting /approve-spec <full head SHA> on the pull request (the author may do so as an engineering owner). The lazy-load contradiction with the shared-touch-module records and the menu's copy of Popover's surface motion are fixed at this head. These changes are still required:
- Miss: the grow is measured from
buttonRef.current(DropdownMenu.tsx:1172), the Button that a finger's press scales to 0.98, while the layer-runtime record's INV22 has a layer use the trigger's untransformed box, so the grow should measure a box the press does not scale. - Miss:
.changeset/motion-rect-morph.mdand.changeset/motion-morph-path.mdstill say no component uses the rect morph while DropdownMenu's grow plays on it, so drop that clause or land #9764 and #9769 first. - At 6,225 authored lines the
Stack:line does not hold, because the heads of #8485 and #8508 are not contained in this head, so restack once #9764, #9769 and #8078 land or make the line hold for every pull request it lists. - The touch-presentations record's platform evidence needs Android emulator runs at this head (animations at normal scale and removed, plus a slow-device profile) and the iPhone runs taken again here, since this head replaces the grow's implementation.
| // Whether the presentation in progress is the touch one, held from the | ||
| // open until its close begins (Layer/presentationHold). | ||
| const [touchHold] = useState(() => createPresentationHold<boolean>()); | ||
| // The hold that keeps taps off the page until the grow's first frame. |
There was a problem hiding this comment.
Non-blocking: this comment says what a clearer name would say. Renaming frameHoldRef to firstFrameTapHoldRef lets the comment go (INV29).
There was a problem hiding this comment.
Approval waits on an engineering owner. This head adds sentences to the DropdownMenu record's FR21 (a pen's open grows the menu, the grow follows the popover's path, taps stay held until the menu shows) beside the touch-presentations record's FR31, and a bullet to the Popover record naming createRectMorphSurfaceMotion and the surface-motion options DropdownMenu passes. The earlier /approve-spec covered a previous head, so an engineering owner records the decision on this text by commenting /approve-spec <full head SHA> on the pull request (the author may do so as an engineering owner). These changes are still required:
- Miss: the grow is measured from
buttonRef.current(DropdownMenu.tsx:1172), the Button that a finger's press scales to 0.98, while the layer-runtime record's INV22 has a layer use the trigger's untransformed box, so the grow should measure a box the press does not scale. - Miss:
.changeset/motion-rect-morph.mdand.changeset/motion-morph-path.mdstill say no component uses the rect morph while DropdownMenu's grow plays on it, so drop that clause or land #9764 and #9769 first. - At 6,229 authored lines the
Stack:line does not hold, because the heads of #8485 and #8508 are not contained in this head, so restack once #9764, #9769 and #8078 land or make the line hold for every pull request it lists. - The touch-presentations record's platform evidence needs Android emulator runs at this head (animations at normal scale and removed, plus a slow-device profile) and the iPhone runs taken again here, since the grow's implementation changed after the head where they were taken.
Stack: #9764 → #9769 → #8485 and #8080; #8078 → #8508 and #8080.
Base: 6c17632 and cab4595, merged as 3b51d59.
User need
Under a finger a DropdownMenu only faded in, while a Popover, like an iOS menu, grows out of its control.
Problem and solution fit
A finger's or a pen's open now grows the menu out of its trigger, and its close shrinks it back (
spec:AST-094FR31). It plays on Popover's surface motion and the shared rect morph, on the popover's path (edgeGrowPath) and timing tokens.A tap's open, at the lift in a scroll view too, counts as a finger's.
The grow ships with DropdownMenu, so every open grows, the first one included. Taps stay held from the open until the menu shows.
Current specification
spec:AST-094FR31 and FR45.component:DropdownMenuFR21: its grow gap closes, and its pan-dismissal gap stays.component:Popovernames the surface motion options DropdownMenu passes.Public delta and caller burden
presentation="popover"keep the layer's entrance.Design and alternatives
Evidence
reducedMotionFade.Writing rubric results
R1 narration, R2 research, R4 rejected alternatives, R5 change references, R6 open questions: none. R3 evidence: FR21's verification row names the grow tests. R7 project status: FR21 keeps its pan-dismissal gap as an implementation gap.
Scope
Components: DropdownMenu, MoreMenu, Popover (its surface motion takes DropdownMenu's options),
useMenuPress(itslongPressDelayMsdefault change comes from #8078).Testing
At c13c351, on 6c17632 and cab4595 (merged as 3b51d59). It is a88dfbb with #8078's main merge merged in, its own lines unchanged, plus one test fix. On the sandbox page "Menu grow cases" (
/sandbox/pages/menu-grow-cases/), on a phone:At c13c351, on an iPhone 14 and a Pixel 8, these steps, Reduce Motion and a cold first open match 781b36e side by side. The cold first open grows at once at both heads, on both phones. Under Reduce Motion the menu fades at full size, within 200 ms. Android emulator runs at this head are pending.
Taps stay held from the open to the grow's first frame, where the grow's own hold takes over. A tap outside is held from 3 to 10 ms after the layer's open request until the grow ends, at 251 to 270 ms. That holds on first and later opens, in Chromium and WebKit.
Under Reduce Motion, the layer's own fade plays: 200 ms on easeInOut, no delay. At c13c351, from its first frame it is full in 194 to 202 ms, in both engines, sampled once per 16.7 ms frame. That frame comes 11 to 20 ms after the layer's open request. At 781b36e, the dip showed one frame at full opacity, then about 100 ms at opacity 0. Its 100 ms fade then made it full 214 to 257 ms after the open request. On the phones at bdb2003, the visible ramp runs from its first changed frame to full opacity. It takes 167 and 166 ms on the Pixel 8, and 134 ms on the iPhone 14. The fade's first frames stay near zero, so the visible ramp measures under the configured 200 ms. At 781b36e, the ramp took 66 to 109 ms on the Pixel and 79 ms on the iPhone, after about 100 ms at opacity 0.
Local gates: the ui project (24,433) and the Node project (19,114) pass. So do the core type check, strict ESLint,
pnpm check:knowledgeandpnpm review:pre. The DropdownMenu, motion, Layer, nav, Popover, BottomSheet, Carousel and SnapPaging interaction specs pass in Chromium: 416 pass, 22 skip. In WebKit's CI container at one worker: 329 pass, 109 skip. Motion's set includesanimatedLayout,animatedLayoutAdopters,animatedLayoutBusy,animatedLayoutRevealPaint,themeTempoandthemeTiming. Their accessibility specs pass in Chromium (297), and the browser ones in WebKit's container (16). Two press-model cases inDropdownMenu.test.tsxstub the presentation's input hold, which jsdom never ends. The browser touch-press specs cover the real hold in both engines.scripts/lazy-sheet-bundle.test.mjsnow pins DropdownMenu's import loading the animator at once, for the grow;rideKeyboardstill waits for the sheet. At 92dd62a, the lazy base, that updated case fails.DropdownMenuTouchFocusRing.a11y.chromium.spec.tsread its held finger's row while the menu grew. At six workers its lift missed the row twice in 20. It now reads the row once the grow rests: 120 of 120.Red first: a new case pins that a finger's first open on a page grows at once. At a831c2c, the lazy head, it fails: the grow waits for its code. At 61b57b9, with a tap's lift open counted as a mouse's, only the scrolling-content tap fails. Under Reduce Motion, the new unit and browser cases and two entrance-hold cases fail, and so does the theme case. Before the hold until the grow's first frame, both tap-hold cases fail.
Review only this PR's change: vjeux/astryx@3b51d59...c13c351
Public and internal API delta
component:Popover:popoverSurfaceMotion.ts:createRectMorphSurfaceMotion(surface)returns aPopoverSurfaceMotionthat grows and shrinks the surface on the shared rect morph.usePopoverSurfaceMotion.ts:PopoverSurfaceMotionOptions, withcreateMotionandholdUntilFirstFrame, andpresent'sisTouch. Popover passes none of them.usePopoverSurfaceMotion({createMotion: createRectMorphSurfaceMotion, holdUntilFirstFrame: true}).useMenuSurfaceMotion.ts,menuSurfaceMotion.ts,menuExit.stylex.tsandreducedMotionPreference.ts, which earlier heads added.