Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsContextMenu (@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. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on a maintainer to approve or replace the governing spec for this addition, because component:DropdownMenu is still a draft record: what must be settled is the data-item header/footer and bottom-sheet level-region requirements this PR writes into FR20, the Sub-menu regions row and DEC-13. Two defects need fixes either way, each with a test that reproduces it:
- In a bottom sheet opened from the keyboard, typing into a level's header field (or the menu's own field at a level) moves focus to Back after the first character, because the keyboard-open focus effect in
DropdownMenu.tsxnow re-runs whenever the caller re-rendersitems. - A level whose
itemsfilter to none becomes a plain row in both presentations (renderDropdownItems.tsx,MenuBottomSheetActionList.tsx), so its field and footer disappear mid-typing and the level can't be reopened while the query stays.
[Automated review]
| const currentSubmenu = submenuPath.at(-1); | ||
| // The opened level as the caller renders it now: its rows, header and | ||
| // footer stay live while it shows. | ||
| const currentSubmenu = resolveSheetLevel(items, submenuPath); |
There was a problem hiding this comment.
Reading the level live makes currentItems (next line) a new array on every render where the caller rebuilds items, and the keyboard-open effect further down ([currentItems, isOpen], ending around line 763) then refocuses the sheet's first button, which at a level is Back. In a sheet opened with Enter, typing "perf" into the action-sheet story's Filter labels field leaves "p" with focus on Back, and the menu's own header field at a level regresses the same way. Keying that effect to the open and the level (submenuPath) instead of currentItems would keep the field focused; a keyboard-open typing test would cover it.
There was a problem hiding this comment.
Fixed in 056b6f6, the same issue the 01:48Z review raised again. The keyboard-open effect now depends on [isOpen, submenuPath], not currentItems, so it runs on the open and on each level change but not on caller renders. A case opens the sheet from the keyboard and types into a level's field key by key; it fails with the previous effect.
| label={option.label} | ||
| isDisabled={option.isDisabled}> | ||
| isDisabled={option.isDisabled} | ||
| header={option.header} |
There was a problem hiding this comment.
This branch only runs while option.items.length > 0 (line 114), so when a header filter matches nothing the row turns into a leaf: the flyout, the field being typed in and the footer unmount, focus drops to the page, and while the caller keeps the query the level can't be reopened. A compound DropdownMenuSubMenu with no rows keeps all three. Treating a row with items and a renderable header or footer as a sub-menu even with zero rows, here and in MenuBottomSheetActionList's isSubmenu, would match it.
There was a problem hiding this comment.
Fixed in e9f7275 and narrowed in ecedadd. renderDropdownItems and MenuBottomSheetActionList both read opensSubMenu(item, {showsRegions}): where sub-menu regions show (DropdownMenu's popover and sheet), a row with header or footer stays a sub-menu while its items is empty. Elsewhere, and without either, an empty items is still a plain row. Cases type a filter that matches nothing, in the popover and in the sheet; both fail with the previous code.
| // A leaf row carries no header or footer of its own. | ||
| expect( | ||
| screen.getByRole('menuitem', {name: 'Rename', ...h}), | ||
| ).not.toHaveAttribute('header'); |
There was a problem hiding this comment.
Nit: Rename has no header, so this passes with or without the strip in renderLeafItem. Giving a leaf row a header and asserting it renders nowhere would prove the strip.
There was a problem hiding this comment.
Agreed, the assertion was vacuous. In 2f03c7c, Rename now has a header. The case checks that no such field renders and that the row carries no header attribute; with renderLeafItem's strip removed, the attribute check fails.
| * @file menuSheetPath.ts | ||
| * @input The menu's live `items` and the bottom sheet's path of opened rows | ||
| * @output `resolveSheetLevel`: the opened level as the caller renders it now | ||
| * @position Internal; used by the DropdownMenu and ContextMenu bottom sheets. |
There was a problem hiding this comment.
Nit: only DropdownMenu.tsx imports this at this head; ContextMenu's sheet doesn't use it yet.
232688c to
fcf3a1d
Compare
There was a problem hiding this comment.
Approval waits on a maintainer to approve or replace the governing spec for this addition: component:DropdownMenu is still a draft record, and what must be settled is the data-item header/footer and bottom-sheet level-region requirements this PR writes into FR20, the Sub-menu regions row and DEC-13. Two defects need fixes either way, each with a test that reproduces it:
- In a bottom sheet opened from the keyboard, typing into a level's header field (or the menu's own field at a level) moves focus to Back after the first character, because the keyboard-open focus effect in
DropdownMenu.tsxre-runs whenever the caller re-rendersitems. - A level whose
itemsfilter to none becomes a plain row in both presentations (renderDropdownItems.tsx,MenuBottomSheetActionList.tsx), so its field and footer disappear mid-typing and the level can't be reopened while the query stays.
[Automated review]
fcf3a1d to
e9f7275
Compare
… its filter matches nothing ContextMenu's popover and bottom sheet read the same opensSubMenu as DropdownMenu's (meta#7839's fix). A ContextMenu bottom-sheet case types a filter that matches nothing, goes Back, and opens the level again with the field as left; it fails with the previous MenuBottomSheetActionList.tsx. The items doc says a row with header or footer stays a sub-menu while items is empty.
There was a problem hiding this comment.
Approval waits on a maintainer to approve or replace the governing spec for this addition: component:DropdownMenu is still a draft record, and what must be settled is the data-item header/footer requirement (including a row staying a sub-menu while its items is empty) and the bottom-sheet level regions this PR writes into FR20, the Sub-menu regions row and DEC-13. The emptied-filter fix holds; two defects need fixes either way, each with a test that reproduces it:
- In a bottom sheet opened from the keyboard, typing into a header field at a level (the level's own, or the menu's own
header) moves focus to Back after the first character, because the keyboard-open focus effect inDropdownMenu.tsxre-runs whenever the caller re-rendersitems; menus that pass no new prop regress too. - A sheet level whose row label changes while it is open, such as a count in "Labels (5)", and that has no
idkeeps its open-time header, so its controlled filter ignores typing; find the row another way, or document and warn that such a row needs anid.
[Automated review]
| // The opened level as the caller renders it now: its rows, header and | ||
| // footer stay live while it shows. | ||
| const currentSubmenu = resolveSheetLevel(items, submenuPath); | ||
| const currentItems = currentSubmenu?.items ?? items; |
There was a problem hiding this comment.
Still open on this revision: at a level currentItems is now a new array on every caller render with inline items, and the keyboard-open effect at lines 729-764 depends on [currentItems, isOpen], so after Enter on the trigger each keystroke in a header field at a level refocuses the sheet's first button, Back. A menu's own controlled header at a nested level keeps "abc" on the base and "a" here. Running that effect on the open and on level navigation (submenuPath) instead, with a case that opens the sheet from the keyboard and types several characters into a level's field, would fix and cover it.
There was a problem hiding this comment.
Fixed in 056b6f6: the keyboard-open focus effect now depends on [isOpen, submenuPath], so it runs on the open and on each level change but not on caller renders, which make a new currentItems at a level read from inline items. New case: open the sheet with Enter, Enter into a level with a controlled filter, focus the field and type "f", "e", "a", letting the sheet's animation frames run after each key. With the previous effect the field keeps "" because focus went back to Back; now it reads "fea" and keeps focus.
| for (const opened of path) { | ||
| const live: DropdownMenuItemData | undefined = | ||
| options != null ? findRow(options, opened) : undefined; | ||
| level = live ?? opened; |
There was a problem hiding this comment.
When a row's label changes while its level shows (a count such as Labels (${visible.length})) and it has no id, no row matches and the level keeps its open-time shape, header included: typing "perf" into that level's controlled filter leaves it empty and the rows unfiltered, while id: 'labels' makes it work. The docs ask for an id only when labels repeat. A positional fallback at this level, or docs plus a development warning for rows whose label can change, would close it.
There was a problem hiding this comment.
Fixed in 056b6f6 with the positional fallback. Each opened row is now kept with its index among its level's rows (openSheetLevel). When no row at the level carries the opened text label any more, as with a count in it, the row at that index is the level, provided it opens a sub-menu. Repeated labels without an id still keep the open-time shape, so they never swap levels; the existing repeated-label case still passes.
New case: a row labelled Labels (${n}), no id, with a controlled filter. Typing "perf" now shows "perf" and filters to Performance, and the heading reads "Labels (1)". With the previous menuSheetPath.ts the field stays empty.
The header JSDoc and the changeset now say an id is needed for repeated labels or rows that reorder.
… its filter matches nothing ContextMenu's popover and bottom sheet read the same opensSubMenu as DropdownMenu's (meta#7839's fix). A ContextMenu bottom-sheet case types a filter that matches nothing, goes Back, and opens the level again with the field as left; it fails with the previous MenuBottomSheetActionList.tsx. The items doc says a row with header or footer stays a sub-menu while items is empty.
There was a problem hiding this comment.
Approval waits on a maintainer to approve or reject the requirements this PR writes into the draft component:DropdownMenu record: data items taking a sub-menu header and footer (and staying a sub-menu while items is empty), the bottom sheet showing a level's own regions in place of the menu's, and how the sheet finds an opened level again, including the new positional match. The keyboard-focus and relabelled-row fixes hold; one new defect needs a fix either way:
- When an opened row without an
idleavesitemswhile its level shows, for example filtered out by the menu's own searchheader, the sheet switches to whichever sub-menu row now sits at that position (menuSheetPath.ts); take the positional match only when it can still be the same row, such as while the level keeps its row count, and add a case for it. - Minor:
ContextMenushares the item type and helpers, so aContextMenurow with aheaderand emptyitemsnow opens an empty sub-menu instead of acting.
[Automated review]
| (sameLabel.length === 0 && atPosition != null && opensSubMenu(atPosition) | ||
| ? atPosition | ||
| : undefined) |
There was a problem hiding this comment.
When the opened row has no id and leaves items while its level shows (say the menu's own search header, which still shows at a level without regions of its own, filters it out), no row carries its label, so this takes whatever sub-menu row now sits at that index: the level's rows, heading and dialog name switch to a sub-menu the person never opened, with no new prop involved. The base and the previous revision keep the opened level. Taking the positional candidate only while the level keeps the row count it had when the row opened (a relabel keeps it, a removal does not) would close it, and a case that filters the opened row out would cover it.
There was a problem hiding this comment.
Fixed in ecedadd. Each path entry now also keeps its level's row count (SheetPathEntry.count). Position is used only for an opened row without an id, when no row carries its label any more, while the level still has that many rows and the row there opens a sub-menu: that is, only when it can still be the same row. Otherwise the level keeps its last known shape.
New case: a sheet whose own search header filters the root rows "Move to", "Labels" and "Copy to". Open Labels, then type "to" into that search, which still shows on Labels. The level stays Labels with "Bug", with ids and without. The previous menuSheetPath.ts switches to "Copy to" and "Drafts" in both. The counted-label and repeated-label cases still pass.
| item.items != null && | ||
| (item.items.length > 0 || | ||
| isRenderable(item.header) || | ||
| isRenderable(item.footer)) |
There was a problem hiding this comment.
ContextMenu also renders through renderDropdownItems and MenuBottomSheetActionList, and ContextMenuItemData aliases DropdownMenuItemData, so a ContextMenu row with header and items: [] now opens an empty sub-menu (no rows, no field) where the base rendered a plain row. Applying the empty-items rule only where regions render would keep ContextMenu as it was.
There was a problem hiding this comment.
Fixed in ecedadd. The rule now applies only where regions render. opensSubMenu(item, {showsRegions}) reads the presentation:
renderDropdownItemspasses it through its recursion;MenuBottomSheetActionListtakes it asshowsLevelRegions;- DropdownMenu's popover and sheet pass
true; ContextMenu and Breadcrumbs leave it off.
So in this PR a ContextMenu row with header and items: [] stays a plain row, as before. A new ContextMenu case checks it in the pointer menu (no aria-haspopup) and in the sheet (a press acts and closes); it fails on the previous head. #7840, where ContextMenu shows sub-menu regions, passes the flag there and turns that case into an empty sub-menu that shows its field.
056b6f6 to
ecedadd
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the requirements this pull request writes into the draft component:DropdownMenu and component:DropdownMenuSubMenu records: data items taking a sub-menu header and footer (and staying a sub-menu while items is empty), a bottom-sheet level showing its own regions in place of the menu's, new FR23's live sheet levels with their row matching, and the sub-menu record's Regions cell and Sheet state. 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. Two small wording fixes apply either way:
- The menu's own
headerandfooterentries inDropdownMenu.doc.mjs, and FR15, still say the bottom sheet shows them at every level, while a level with regions of its own shows neither. - The
DropdownMenuSubMenurecord's new Sheet state describes a level only a data item can open, since a compound menu never uses the bottom sheet.
[Automated review]
The bottom sheet now has meta#7839's structure on main's sheet (its view heading and level regions); the action-sheet card stays the branch taken for plain actions on a compact touch screen, and every other menu keeps that sheet. The card no longer registers the springs or alert tables: the alert's motion values are internal constants its motion reads straight from the tables (meta#8084), so registering them would register nothing and no theme variable retunes them.
…t level with its own, and the Sheet state names a data item's level Two wording points from the review on 5745647. The menu's own header and footer entries in DropdownMenu.doc.mjs, and FR15, still said the bottom sheet shows them at every level, while FR20 has a level's own regions replace both: the doc entries now say so, and FR15 says it with a pointer to FR20 (its closing sentence loses a restatement so the cell keeps its column width). The DropdownMenuSubMenu record's Sheet state now names a data item's level, since a compound menu never uses the bottom sheet. Record and doc wording only.
… its filter matches nothing ContextMenu's popover and bottom sheet read the same opensSubMenu as DropdownMenu's (meta#7839's fix). A ContextMenu bottom-sheet case types a filter that matches nothing, goes Back, and opens the level again with the field as left; it fails with the previous MenuBottomSheetActionList.tsx. The items doc says a row with header or footer stays a sub-menu while items is empty.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the requirements this pull request writes into the draft component:DropdownMenu and component:DropdownMenuSubMenu records: data items taking a sub-menu header and footer (and staying a sub-menu while items is empty), a bottom-sheet level showing its own regions in place of the menu's (FR15, FR20, DEC-13), new FR23's live sheet levels with their row matching, the sub-menu record's Regions cell and Sheet state, and whether this data spelling of DropdownMenuSubMenu's regions stands under architecture:public-component-api INV21 ("One API per job"). 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. One fix applies either way: architecture:public-component-api INV18, now on main, asks that a rendered content slot be a part named in the component's anatomy, so the records need anatomy and theming-map entries for the sub-menu and sheet-level header and footer regions.
[Automated review]
| * it shows, or when the rows can reorder or grow. Ignored on a row without | ||
| * `items`. | ||
| */ | ||
| header?: ReactNode; |
There was a problem hiding this comment.
header and footer here are rendered content slots. architecture:public-component-api INV18, now on main, asks that such a slot be a part named in the component's anatomy, and neither component:DropdownMenu's anatomy nor component:DropdownMenuSubMenu's names a sub-menu or sheet-level header or footer. Naming those parts, with theming-map entries, in the records settles it with no API change.
meta#8220's dependencies in one commit, so its compare link shows only its own change: meta#8089 and meta#7839, the sub-menu's header and footer. The spec table takes meta#7839's row changes (Sub-menu regions, FR15, FR20, FR23 and their verification rows) on top of the rows the stack already carries, none of which meta#7839 touches.
Main's meta#8515 added DropdownMenu FR21 (the menu grows out of its trigger under a finger) beside FR20; the requirements table keeps this branch's FR20 wording and FR23 row around it.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the requirements this pull request writes into the draft component:DropdownMenu and component:DropdownMenuSubMenu records: data items taking a sub-menu header and footer (and staying a sub-menu while items is empty), a bottom-sheet level showing its own regions in place of the menu's (FR15, FR20, DEC-13), new FR23's live sheet levels with their row matching, the sub-menu record's Regions cell and Sheet state, and whether this data spelling of DropdownMenuSubMenu's regions stands under architecture:public-component-api INV21 ("One API per job"). 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. One fix applies either way: architecture:public-component-api INV18 asks that a rendered content slot be a part named in the component's anatomy, so the records need anatomy and theming-map entries for the sub-menu and sheet-level header and footer regions.
[Automated review]
…nu sub-menu regions Main's meta#8648 put ContextMenu on the public headless context menu (FR10) and rewrote the record's ownership lines. Her text is kept as merged; this branch adds back only its own surface-host wording (the ownership line, FR2 and its verification row) and proposed FR12 after FR10.
|
I'm pausing this work for now to reduce contention on the repo. Thank you for the reviews and the spec guidance. |
User need
What led to it: this comes from moving every menu and picker in our app onto Astryx, which began with a press, slide and lift bug in its phone menus. Our session menu's "Labels" sub-menu, built from
items, has a filter field and a "Create label" button, and on a phone they were missing.Why:
DropdownMenuSubMenutakes aheaderandfooter, but a data item can't, and the bottom sheet shows the menu's own regions at every level, so a level's own controls never appear on a phone.Problem and solution fit
A data item with
itemstakesheaderandfooter, and the popover shows them around that sub-menu. In the bottom sheet, a level whose row has either shows them around its rows in place of the menu's. Each level's regions mount on their own, so typed text never carries between levels.Current specification
component:DropdownMenu(draft): FR20 and DEC-13, proposed with #7530, now name data items and the sheet's levels, and a new FR23 says every sheet level follows the currentitems.component:DropdownMenuSubMenu(draft) names the sheet's levels. #7840 extends FR20 toContextMenu.Public delta and caller burden
headerandfooteron a data item withitems. With either, the row stays a sub-menu even whenitemsis empty, so a filter that matches nothing keeps its field.itemswhile it shows; rows that share or change a label, or reorder, need anid.Design and alternatives
items. Smaller, but a menu-wide search couldn't filter nested rows.items(chosen). The sheet is already a modal dialog, so a level's regions sit in it directly. The cost: callers whose rows share or change labels add anid.Evidence
New cases in
DropdownMenuSubMenu.test.tsxandContextMenu.test.tsxcover the data items, the sheet's level regions and live levels;SubMenuHeaderFooter.a11y.chromium.spec.tschecks the sheet in Chromium. They fail onmain.Writing rubric results
R1 narration: none.
R2 research: none.
R3 evidence: none.
R4 rejected alternatives: none.
R5 change references: none.
R6 open questions: none.
Evidence
See Testing.
Scope
Testing
In Storybook, open Core/DropdownMenu → Submenu / header and footer, action sheet and open "Session".