Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:spec-owner-review
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsContextMenu (@astryxdesign/core) · View in Storybook
MoreMenu (@astryxdesign/core) · View in Storybook
Component 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.
|
| Component | JavaScript | CSS |
|---|---|---|
| core/DropdownMenu | +19.3KB |
+727B |
| core/MoreMenu | +19.4KB |
+727B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.6KB | 35.1KB | +557B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
55 of 502 shot(s) changed.
A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.
| component | story | theme | mode | pixels |
|---|---|---|---|---|
| Selector | Indicator Space Bottom Sheet Narrow End RTL | neutral | light | 753,933 |
| Selector | Indicator Space Bottom Sheet Wide Start | neutral | light | 740,072 |
| RichTextEditor | Markdown parity: side by side | probe | dark | 558,840 |
| RichTextEditor | Markdown parity: side by side | probe | light | 540,756 |
| Chat | Full AI Chat | probe | light | 230,823 |
| Chat | Full AI Chat | probe | dark | 229,805 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Collapsible | Dividers | probe | dark | 66,445 |
| Collapsible | Dividers | probe | light | 66,445 |
| AppShell | Playground | neutral | dark | 65,198 |
| AppShell | Playground | neutral | light | 63,788 |
| Chat | Default | neutral | dark | 36,304 |
| Chat | Default | neutral | light | 35,767 |
| Chat | Empty State | probe | light | 23,531 |
| Chat | Empty State | probe | dark | 22,622 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | dark | 15,378 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | light | 14,107 |
| Chat | With Attachments | probe | light | 13,406 |
| Chat | With Attachments | probe | dark | 11,232 |
and 35 more.
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report
![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 spec decision before it can be approved. The in-place sub-menu drill-in has no approved spec, and the new draft AST-094 touch-presentation spec and the proposed DropdownMenu (FR8, FR20, FR23) and useMenuPress (FR9, DEC-8) edits are not yet approved; AST-094 also changes current theme-token, user-state and scroll-chaining rules. An engineering owner can record that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision is, these need changes:
- The pull request compares 31,481 authored lines against
mainbecause it carries its open dependencies; once they land or it is rebased onto them, it should contain only its own 1,402-line commit. - The drilled-in header shows the row's label but is named
Back to <parent>, which fails WCAG 2.5.3 (Label in Name), so its name should start with the visible text. .changeset/dropdownmenu-submenu-in-place.mdneeds a[feat]tag and apatchbump, since CI's lint job fails on it.- After drilling two levels and going back one, the first sub-menu loses its parent copy (
menuDrillInMotion.ts:381). useMenuPress.spec.mdstill says 500 ms while the code ships 150 ms.
[Automated review]
| xstyle={rtlStyles.mirror} | ||
| /> | ||
| label={stringLabel ?? backLabel} | ||
| aria-label={backLabel} |
There was a problem hiding this comment.
The header shows label ("Move to") but aria-label names it "Back to Actions", so its name does not contain the visible text (WCAG 2.2 SC 2.5.3, Label in Name). A speech-input user who says "Move to" does not reach it. Start the name with the visible label (for example "Move to, back to Actions"), or keep the visible "Back to" text.
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| '@astryxdesign/core': minor | |||
There was a problem hiding this comment.
pnpm check:changesets fails on this file because the body needs a [category] tag. While packages are on 0.x a compatible feature is also a patch bump, so use '@astryxdesign/core': patch and start the body with [feat].
| } | ||
| if (kind === 'push') { | ||
| // The parent card behind: the popover as it stands now. | ||
| removeParent(); |
There was a problem hiding this comment.
Every push removes the existing parent copy here, and every pop removes the only one (lines 431 and 506). After Root → Move to → Archive → back, "Move to" shows with nothing behind it, unlike the first time it opened. Keep one copy per pushed level, and on pop remove only the top one.
1c3307b to
df81a27
Compare
df81a27 to
01a6f26
Compare
… Mobile Safari) and its Chromium storyboard
There was a problem hiding this comment.
This needs an engineering owner's spec decision before it can be approved: the in-place sub-menu drill-in has no approved spec, and the new draft AST-094 touch-presentation spec and the proposed DropdownMenu (FR8, FR20, FR23) and useMenuPress (FR9, DEC-8) edits are not yet approved. 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 decision, these still need changes:
- The compare carries 31,889 authored lines from its open dependencies; once they land, or after a rebase onto them, it should contain only the 1,810-line drill-in commit.
- The drilled-in header shows the row's label but is named "Back to ", which fails WCAG 2.5.3 (Label in Name), so its name should start with the visible text.
.changeset/dropdownmenu-submenu-in-place.mdneeds a[feat]tag and apatchbump, since CI's lint job fails on it.- After drilling two levels and going back one, the first sub-menu has no parent card behind it (
menuDrillInMotion.ts:518and:650). useMenuPress.spec.mdstill lists a 500 ms long-press delay while the code ships 150 ms, and AST-094 amendsspec:AST-090, which exists only in #8003.
[Automated review]
| @@ -140,6 +140,7 @@ Consumer migration instructions belong in consumer docs and release notes. | |||
| | stray-click window | 400 ms, or the next `pointerdown` | how long after a tracked release the browser's click is still swallowed | `MENU_PRESS_STRAY_CLICK_MS` | `module:DropdownMenu/useMenuPress` | stable | | |||
| | settle time | 300 ms | proposed: how long an opening press must last before its release may act | `MENU_PRESS_SETTLE_MS` | `module:DropdownMenu/useMenuPress` | proposed | | |||
| | long-press delay | 500 ms | proposed: how long a finger rests on the trigger before the menu opens under it | `longPressDelayMs` | `module:DropdownMenu/useMenuPress` | proposed | | |||
There was a problem hiding this comment.
This row still gives the long-press delay as 500 ms, while useMenuPress.ts now defaults longPressDelayMs to 150 ms (300 ms inside a scroller), and this pull request edits FR9 and adds DEC-8 in the same record. State the shipped value here, or keep the code at 500 ms, so an owner approving the record approves what ships.
| 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.
spec:AST-090 is not in the repository at this head or on main; it exists only in the open #8003, which the stack list does not name. List that pull request as a dependency, or drop the FR1/FR10 amendment until AST-090 lands, so an owner can read what FR7 departs from.
01a6f26 to
1e86c46
Compare
There was a problem hiding this comment.
This needs an engineering owner's spec decision before it can be approved: the in-place sub-menu drill-in has no approved spec, the new draft AST-094 touch-presentation spec and the proposed DropdownMenu (FR8, FR20, FR23) and useMenuPress (FR9, DEC-8) edits are not yet approved, and AST-094 would change current scroll-chaining, theme-token, user-state and button-press rules that an owner would have to update. 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 decision, these still need changes:
- The compare carries 31,889 authored lines from its open dependencies; once they land, or after a rebase onto them, it should contain only the 1,810-line drill-in commit.
- The drilled-in header shows the row's label but is named "Back to ", which fails WCAG 2.5.3 (Label in Name), so its name should contain the visible text.
- After drilling two levels and going back one, the first sub-menu has no parent card behind it (
menuDrillInMotion.ts:518and:650). useMenuPress.spec.mdstill lists a 500 ms long-press delay while the code ships 150 ms, and AST-094 amendsspec:AST-090, which exists only in #8003.
[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.
|
…), on the live card
1e86c46 to
98cf5d3
Compare
github-actions
Bot
added
the
needs:design-review
…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.
This pull request sat on meta#8076 and carried its old copies of the harness meta#8045 and meta#8046 landed on main. meta#8360 carries meta#8076 with main's harness, so after the merge this pull request's own diff is the presentation resolver's commits. The one conflict, the gestures README, keeps both sides: this pull request's paragraph on the presentation prop ends the touch presentation section, and meta#8360's migration section follows it.
…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.
… as a dependency meta#8627 is now one commit on main; meta#8360 carries meta#8076, meta#8073 and meta#8446. useMenuPress.ts: the trigger half lives in menuTriggerPress.ts, so it takes meta#8360's trigger changes there. The trigger's holds run on the long-press detector and pass FR29's 13 px drift allowance as each press starts, under every primary pointer (FR43). The hold duration comes from motion/uikit/longPress.ts. MENU_PRESS_TRIGGER_DRIFT_PX is re-exported beside MENU_PRESS_LONG_PRESS_MS. The stray-click window is the shared click guard's, from meta#8360.
meta#8089 now takes meta#8080, which takes meta#8078 (with meta#8627, meta#8360 and meta#8298) and meta#8186. DropdownMenuTouch.ts registers UIKit's two menu springs for the grow, from meta#8080, and keeps the springs table for the in-place sub-menu; its header says so. The usage text takes meta#8080's wording, meta#8089's popover sentence and this PR's in-place sub-menu sentence.
…rings and no other (red) architecture:component-style-authoring INV6: a touch layer ships the springs it plays. The grow and collapse already carry UIKit's two menu springs and no other row. The in-place sub-menu reads the whole springs table and registers all of it, so its bundle carries every row: the keyboard's spring among them. This case bundles createMenuDrillInMotion alone and fails here.
…prings and no other The in-place sub-menu plays springs.subMenuOpen and springs.subMenuBack. It now imports those two springs instead of the whole table, and registers them alone through registerSubMenuSprings, as the grow does with registerMenuSprings. Its bundle carries those two springs and no other row (architecture:component-style-authoring INV6).
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision: on a phone this pull request opens a sub-menu in place over the parent's rows, which current spec:AST-094 FR34 rules out and OQ10 leaves to an owner, and its header springs and its 10 extra open-menu elements need AST-094 FR4 and an INV7 / AST-017 FR60 exception settled as well. An engineering owner records that decision, with FR4, FR34 and OQ10 amended to match, by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Independently of that decision, these need changes:
- A back tapped while the card is still growing restarts from the end of the push instead of from where the box is drawn (
menuDrillInMotion.ts,capture; AST-094 FR6). scripts/touch-support-bundle.test.mjsfails for the root and hooks entries, which now carrygestures/nativeScroll.tsthroughhooks/menuTriggerPress.ts.- The compare is 37,267 authored lines because it carries the unmerged dependency pull requests; this pull request's own change is 2,221 lines and should land on top of them, decomposed or justified.
[Automated review]
…andoned meta#7839 entered this branch's dependencies in six merges, the last of meta#7839 53e8deb. Its whole diff against the main it was built on (d5cd551), 14 files, comes off in this one commit, so the branch ships none of its code. The history stays. This PR's own change needs none of it: no file here imports menuSheetPath.ts or opensSubMenu.ts, and the core typecheck and the DropdownMenu, ContextMenu and MoreMenu suites pass without it. The DropdownMenu spec loses meta#7839's rows (FR23, its FR20 and FR15 text, the sub-menu regions row), so the FR21-before-FR23 order that merge kept no longer applies.
…sub-menu springs [skip ci] createMenuDrillInMotion's comment still said it registers the springs table; since the sub-menu ships only its two springs, it registers those (registerSubMenuSprings), as createMenuSurfaceMotion's comment says of the menu springs.
… as a dependency [skip ci] meta#8220 no longer carries the withdrawn meta#7839. This PR's action-sheet card used two things only meta#7839 provided: its opensSubMenu helper, and header and footer on a data row. isPlainAction now checks for a nested level itself, by main's rule (a non-empty items), and drops the per-row header and footer checks, since rows have neither. The level's own header and footer still keep the sheet (hasRegions). The DropdownMenu bottom sheet takes main's regions again, inside this PR's card-or-sheet branch. DropdownMenuTouch.ts names both register functions it calls, and says the card plays the alert's two springs through useAlertMotion (meta#8373). The presentation description keeps this PR's card sentence and meta#8080's wording.
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision: on a phone this pull request opens a sub-menu in place over the parent's rows, which current spec:AST-094 FR34 rules out (its rows and a Back to <parent> row replace the menu's) and OQ10 leaves to an owner, and the same decision has to cover the header's springs (FR4), ten more elements under a finger than under a mouse (architecture:component-style-authoring INV7, spec:AST-017 FR60), and the new architecture:layer-runtime exit-copy text and spec:AST-057 motion-role sentence. 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 decision, these still need fixing:
- A back tapped while the card is still growing jumps to the end of the push before it shrinks (
menuDrillInMotion.ts:398, AST-094 FR6). main'stouch-support-bundletest fails at this head because the publicuseMenuPressnow reachesgestures/nativeScroll.ts(hooks/menuTriggerPress.ts:24).- The compare is 36,206 authored lines; land or retarget onto the dependencies so it carries only this change, and split or justify what remains over 2,000.
[Automated review]
| const capture: MenuDrillInMotion['capture'] = (kind, row, atRoot) => { | ||
| const {surface, list} = getElements(); | ||
| captured = null; | ||
| endTransition(); |
There was a problem hiding this comment.
This disposes every animator of the transition in flight before boxOf(paint) below reads the layout, so a back tapped while the card is still growing first jumps the box to the pushed size and then shrinks from rest. spec:AST-094 FR6 asks for the catch at the value on screen, from the curve, keeping the velocity. Read the running box, card and parent animators' get and getVelocity before ending them, as menuSurfaceMotion.ts does when the menu is reopened during its close.
|
|
||
| import {useCallback, useMemo, type RefObject} from 'react'; | ||
| import {currentGesture} from '../Layer/gestureCounter'; | ||
| import {createNativeScrollProbe} from '../gestures/nativeScroll'; |
There was a problem hiding this comment.
The public useMenuPress reaches this module through useDropdownMenuPress, so the hooks and root entries now carry gestures/nativeScroll.ts, and scripts/touch-support-bundle.test.mjs ("is absent from the root entry", "is absent from the hooks entry") fails at this head, as the description says. Either keep the probe out of what the public hook reaches or carry #8671's test change under an INV13 reading an owner accepts.
Depends on #8089. Review only this PR's change: vjeux/astryx@e01c0d1...837b8e2
User need
On a phone, a DropdownMenu sub-menu swaps the menu's rows for a Back row and its own rows in one frame, so the person loses their place. iOS opens it in place: a card grows out of its row over the parent's dimmed rows, and its header takes them back.
What led to it: comparing our menus with UIKit's on the iOS 26.2 simulator, frame by frame (
spec:AST-094FR34).Problem and solution fit
Under a finger on a phone, tapping a sub-menu row grows a card from it on UIKit's spring (0.4 s); the parent's rows recede (half opacity, 97% scale) and stop taking input. The header shows the row's label; tapping it plays the card back. Reduce Motion cross-fades. A mouse keeps today's drill-in; flyouts are unchanged.
Current specification
spec:AST-094FR34 (current), on the springs FR4 defines (#8076);component:DropdownMenuFR11 and FR20 keep its focus.Public delta and caller burden
No API change. The header is named "Move to, Back to Actions" for assistive technology. Cost per DropdownMenu or MoreMenu import: 3.4 KB JS and 0.7 KB CSS, gzipped.
Elements rendered by the Submenu story in Chromium (
architecture:component-style-authoringINV7):The 5 extra elements are the header's icon and its title's style boundary; a finger already exceeded the mouse before (159 vs 154), so this needs the owner's decision.
Design and alternatives
Evidence
iOS 26.2 simulator, Safari, tap "Move to", then its header:
Android Chromium 158 emulator:
Scope
Testing
On the DropdownMenu "Submenu" story, phone-sized, with touch emulation:
Automated: the drill-in motion unit tests,
DropdownMenuSubmenuDrillIn(Chromium, WebKit) and the sub-menu accessibility specs. The in-place sub-menu registers only its two springs, as the grow does; a bundle test of it failed before (it carried every spring row). Main'stouch-support-bundletest fails here until #8671 makes it per component; #8671's check passes on this head.The local gates are the evidence; no CI run is expected for this push.
Stack (merge in this order): #8360, #8298 and #8627 (any order) → #8671 → #8078 and #8186 (any order) → #8080 → #8089 → this PR. #7839 was withdrawn: one commit reverts its code from this branch, and nothing here depends on it.