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
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This change needs a maintainer decision before it can be approved: the new indicator prop on DropdownMenuRadioGroup adds public API and a new visible behavior, and no current spec covers it yet, because the DropdownMenu record is still a draft and the proposed concept row, FR15 and DEC-8 cannot settle it on their own. A maintainer needs to provide or identify the governing spec, deciding whether the prop is admitted, where the check sits, how the check glyph maps to theme targets, and whether ContextMenuRadioGroup, which re-exports this component, carries it too. Two small fixes apply either way: the FR15 evidence cell says the CheckIndicator record names menu rows as a host, but that record only describes a generic host, and the changeset and description should mention that ContextMenuRadioGroup gains the prop.
[Automated review]
| | FR12 | In the pointer menu ArrowDown on the last enabled row wraps to the first and ArrowUp on the first to the last; PageDown and PageUp move to the last and first fully visible enabled row and, pressed there again, one viewport further without wrapping; ArrowUp on the trigger opens with the last enabled row highlighted; the key that opened the menu and its auto-repeats do not activate; typeahead matches the row's label element alone and ignores Control/Command chords and input-method composition. | Proposed in this change; `DropdownMenu.test.tsx` keyboard suite, `useListFocus.test.tsx`, `useTypeahead.test.tsx` | Proposed; verified in jsdom, pending owner review | | ||
| | FR13 | On a mouse, a nested flyout stays open while the pointer is inside the triangle from where it left its row to the flyout's near edge — including while the pointer is paused there — and closes after the existing delay once the pointer has left both the row and that triangle. The row's click toggle and its guard window are unchanged. | Proposed in this change; `DropdownMenuSubMenu.test.tsx` safe-triangle suite, `useMenuHover.test.tsx` | Proposed; verified in jsdom, pending owner review | | ||
| | FR14 | A compound `DropdownMenuGroup` is one `role="group"` named by its heading through `aria-labelledby`; the heading carries `dropdown-menu-section-heading`, is plain text rather than a menu item, and is skipped by roving focus and typeahead. An untitled group is an unnamed `role="group"`. | DEC-1, docs, and tests | Proposed; awaiting owner approval | | ||
| | FR15 | A `DropdownMenuRadioGroup` with `indicator="check"` MUST draw the theme's `check` indicator, carrying `dropdown-menu-radio`, at the inline end of its chosen row and no mark on its other rows; every row keeps `role="menuitemradio"` and `aria-checked`. The default `radio` draws the radio circle on every row. | Proposed DEC-8; `component:CheckIndicator` names menu rows as a host; Selector's `indicatorPosition` defaults to `end` | Proposed; verified in jsdom and Chromium, pending owner review | |
There was a problem hiding this comment.
The CheckIndicator record doesn't name menu rows as a host: it describes a generic host and lists "Row placement and logical-edge layout" as owned by the host component. Menu rows are named in the CheckIndicator.tsx header comment and the Indicator docs, not in the record. Consider dropping this citation or restating what the record says.
There was a problem hiding this comment.
Fixed in b624158. The citation was wrong: the CheckIndicator record names no particular host. FR15's basis and the PR description now say what the record does say, that row placement and logical-edge layout belong to the host component. That makes the inline-end placement this record's own decision, recorded in DEC-8 and left for the owner to settle.
| * stay `menuitemradio` with `aria-checked` either way. | ||
| * @default 'radio' | ||
| */ | ||
| indicator?: DropdownMenuRadioIndicator; |
There was a problem hiding this comment.
ContextMenuRadioGroup re-exports this component, so ContextMenu callers get indicator too. The description and changeset only name DropdownMenuRadioGroup; stating the ContextMenu exposure lets the maintainer decision cover both names.
There was a problem hiding this comment.
Done in b624158. The prop's JSDoc, the component doc and the changeset now say that ContextMenuRadioGroup is this component under another name and takes indicator too. The PR description's public delta names it as well, so the decision covers both names.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewComponent 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 |
There was a problem hiding this comment.
This change still needs a maintainer decision before it can be approved: the DropdownMenu record is a draft, so a maintainer must provide or identify the governing spec for the new indicator prop, settling whether it is admitted, the inline-end placement, how the check glyph maps to theme targets, and what unchosen rows show when a theme replaces the check indicator. The earlier citation and ContextMenu fixes are in, and two small fixes apply either way:
BreadcrumbMenuRadioGroupre-exports the same component, so the changeset, JSDoc and docs should name it next toContextMenuRadioGroup.- FR15 and the docs say unchosen rows draw nothing, which holds only for the default check indicator: a theme that swaps
checkfor one that draws an unchecked state, such asRadioIndicator, shows a mark on every row.
[Automated review]
| * stay `menuitemradio` with `aria-checked` either way. `ContextMenuRadioGroup` | ||
| * is this component under another name, so it takes the prop too. |
There was a problem hiding this comment.
BreadcrumbMenuRadioGroup re-exports this component too (Breadcrumbs/index.ts), so Breadcrumbs menus also get indicator. Naming it next to ContextMenuRadioGroup here, in the changeset, and in the en and zh docs lets the maintainer decision cover every public name.
There was a problem hiding this comment.
Done in ab9cfab. BreadcrumbMenuRadioGroup is now named next to ContextMenuRadioGroup in the prop JSDoc, the changeset, the en, zh and dense docs, FR15 and the PR description, so the decision covers all three public names.
| | FR12 | In the pointer menu ArrowDown on the last enabled row wraps to the first and ArrowUp on the first to the last; PageDown and PageUp move to the last and first fully visible enabled row and, pressed there again, one viewport further without wrapping; ArrowUp on the trigger opens with the last enabled row highlighted; the key that opened the menu and its auto-repeats do not activate; typeahead matches the row's label element alone and ignores Control/Command chords and input-method composition. | Proposed in this change; `DropdownMenu.test.tsx` keyboard suite, `useListFocus.test.tsx`, `useTypeahead.test.tsx` | Proposed; verified in jsdom, pending owner review | | ||
| | FR13 | On a mouse, a nested flyout stays open while the pointer is inside the triangle from where it left its row to the flyout's near edge — including while the pointer is paused there — and closes after the existing delay once the pointer has left both the row and that triangle. The row's click toggle and its guard window are unchanged. | Proposed in this change; `DropdownMenuSubMenu.test.tsx` safe-triangle suite, `useMenuHover.test.tsx` | Proposed; verified in jsdom, pending owner review | | ||
| | FR14 | A compound `DropdownMenuGroup` is one `role="group"` named by its heading through `aria-labelledby`; the heading carries `dropdown-menu-section-heading`, is plain text rather than a menu item, and is skipped by roving focus and typeahead. An untitled group is an unnamed `role="group"`. | DEC-1, docs, and tests | Proposed; awaiting owner approval | | ||
| | FR15 | A `DropdownMenuRadioGroup` with `indicator="check"` MUST draw the theme's `check` indicator, carrying `dropdown-menu-radio`, at the inline end of its chosen row and no mark on its other rows; every row keeps `role="menuitemradio"` and `aria-checked`. The default `radio` draws the radio circle on every row. | Proposed DEC-8; `component:CheckIndicator` leaves row placement to its host; Selector's `indicatorPosition` defaults to `end` | Proposed; verified in jsdom and Chromium, pending owner review | |
There was a problem hiding this comment.
"No mark on its other rows" holds only for the default CheckIndicator, which renders nothing when unchecked. The row renders the theme's check indicator on every row, so a theme that swaps in one that draws an unchecked state (the documented RadioIndicator swap) shows it at the inline end of every row, as Selector's AV1 allows for Selector. Once that behavior is settled, this row and the docs should say what actually renders.
There was a problem hiding this comment.
Fixed in ab9cfab. FR15, DEC-8, the concept row, the docs, the story and the changeset now say what renders. Every row renders the theme's check indicator at its inline end, in that row's state. The default check draws on the chosen row only. A theme whose check draws an unchecked state, like the documented RadioIndicator swap, shows it on every row, as Selector's AV1 allows. A new unit case renders the group under a theme with indicators: {check: RadioIndicator} and checks that every row draws a radio carrying the menu target and only the chosen one is filled.
There was a problem hiding this comment.
This change needs a maintainer decision before it can be approved: the DropdownMenu record is still a draft, so a maintainer must provide or identify the governing spec for the new indicator prop on DropdownMenuRadioGroup, which ContextMenuRadioGroup and BreadcrumbMenuRadioGroup also expose. That decision needs to settle whether the prop is admitted, the inline-end placement on every pointer type, how the check glyph maps to theme targets, and whether unchosen rows show a replacement check indicator's unchecked state. The earlier disclosure and wording fixes are in and the implementation matches its description, including an unchanged default radio rendering, so nothing else is needed from you until that decision lands.
[Automated review]
|
Decision (ENGOWNER), settling the four open questions: |
There was a problem hiding this comment.
This change still needs a maintainer to record its governing spec before it can be approved. The ENGOWNER decision posted on this PR states the intended direction for the new indicator prop, but DropdownMenu.spec.md is still a draft, so FR15, the Radio mark concept and DEC-8 are not yet current authority. A maintainer needs to commit that decision as current in the owning record, or point to an existing current record that covers it; the record's "Radio indicator" anatomy row and FR2 still describe only the radio circle and should then also describe the check glyph that carries dropdown-menu-radio. The implementation matches the PR description and the default radio rendering is unchanged, so no code change is needed while that decision is recorded.
[Automated review]
| | FR12 | In the pointer menu ArrowDown on the last enabled row wraps to the first and ArrowUp on the first to the last; PageDown and PageUp move to the last and first fully visible enabled row and, pressed there again, one viewport further without wrapping; ArrowUp on the trigger opens with the last enabled row highlighted; the key that opened the menu and its auto-repeats do not activate; typeahead matches the row's label element alone and ignores Control/Command chords and input-method composition. | Proposed in this change; `DropdownMenu.test.tsx` keyboard suite, `useListFocus.test.tsx`, `useTypeahead.test.tsx` | Proposed; verified in jsdom, pending owner review | | ||
| | FR13 | On a mouse, a nested flyout stays open while the pointer is inside the triangle from where it left its row to the flyout's near edge — including while the pointer is paused there — and closes after the existing delay once the pointer has left both the row and that triangle. The row's click toggle and its guard window are unchanged. | Proposed in this change; `DropdownMenuSubMenu.test.tsx` safe-triangle suite, `useMenuHover.test.tsx` | Proposed; verified in jsdom, pending owner review | | ||
| | FR14 | A compound `DropdownMenuGroup` is one `role="group"` named by its heading through `aria-labelledby`; the heading carries `dropdown-menu-section-heading`, is plain text rather than a menu item, and is skipped by roving focus and typeahead. An untitled group is an unnamed `role="group"`. | DEC-1, docs, and tests | Proposed; awaiting owner approval | | ||
| | FR15 | A `DropdownMenuRadioGroup` with `indicator="check"` MUST render the theme's `check` indicator, carrying `dropdown-menu-radio`, at the inline end of every row, in that row's checked or unchecked state; the default `check` draws only on the chosen row, and a theme whose `check` draws an unchecked state (a radio) shows it on every row, as Selector's AV1 allows. Every row keeps `role="menuitemradio"` and `aria-checked`. The default `radio` draws the radio circle on every row. `ContextMenuRadioGroup` and `BreadcrumbMenuRadioGroup` are the same component. | DEC-8; `component:CheckIndicator` leaves row placement to its host; Selector's `indicatorPosition` defaults to `end` | Accepted; verified in jsdom and Chromium | |
There was a problem hiding this comment.
FR15 now reads "Accepted", but this record is still authority: draft with approved_by: null, so FR15 is not current authority yet. Once the decision is committed as current, FR2 ("every target remains on its current painted element") and the "Radio indicator" anatomy row should also describe the check glyph that carries dropdown-menu-radio in check mode.
There was a problem hiding this comment.
Updated FR2 and the Radio indicator anatomy row in a17e4d9: with indicator="check" the dropdown-menu-radio target sits on the check glyph, and both rows now say so and point at FR15. On authority, FR15's "Accepted" records the engineering-owner decision posted on this PR, the same way the draft ContextMenu record marks its accepted FR6. Promoting the whole DropdownMenu record to current would also turn its other proposed rows into authority. That is the record owner's step, outside this PR's one intent, so this PR leaves the record draft.
There was a problem hiding this comment.
This change still needs a maintainer to record its governing spec before it can be approved. The ENGOWNER decision is now written into DropdownMenu.spec.md, but that record is still a draft, so the Radio mark concept, FR15 and DEC-8 are not yet current authority. A maintainer needs to make that decision current in the owning record, which also means settling the record's other proposed rows, or point to an existing current record that covers the indicator prop; at that point, the design-relationships sentence saying the radio element carries Indicator's shared radio target should also describe the check glyph. The implementation matches its description and the default radio menu renders unchanged, so you don't need to change any code in the meantime.
[Automated review]
| | Caller-rendered item start content | Presents arbitrary React content directly in an action-row start slot. | Caller-supplied content | Context-dependent | FR1, FR3 | | ||
| | Checkbox indicator | Draws the decorative checkbox state for a checkbox action row. | `component:Indicator` | Supporting | FR1, FR3 | | ||
| | Radio indicator | Draws shared radio chrome with the current menu-specific radio target. | Current source and Indicator | Supporting | FR1, FR2, FR3 | | ||
| | Radio indicator | Radio chrome, or with `indicator="check"` the check glyph; menu radio target. | Current source and Indicator | Supporting | FR1–FR3, FR15 | |
There was a problem hiding this comment.
In check mode this part is the glyph CheckIndicator renders through Icon, so it also carries astryx-icon. The sentence under this table still says "The radio element also carries Indicator's shared radio target". When the decision is made current, that sentence should describe the check glyph too.
There was a problem hiding this comment.
Done in 0a9a14c. The note under the anatomy table now says that in a radio group with indicator="check" the part is the check glyph, which carries Icon's astryx-icon rather than Indicator's shared radio target. Making the record current is still the owner's step, outside this PR.
There was a problem hiding this comment.
This change still needs a maintainer to make its governing spec current before it can be approved: the ENGOWNER decision is written into DropdownMenu.spec.md (the Radio mark concept, FR15, DEC-8 and the FR2 and anatomy amendments), but that record is still a draft, so the new indicator prop on DropdownMenuRadioGroup, ContextMenuRadioGroup and BreadcrumbMenuRadioGroup has no current authority yet. A maintainer needs to commit that decision as current in the owning record, which for this record also means settling its other proposed rows, or point to an existing current record that covers the prop. The implementation matches its description and the default radio rendering is unchanged; the one small follow-up is the "Radio indicator" anatomy entry in DropdownMenu.doc.mjs, which should also describe the check glyph that carries dropdown-menu-radio in check mode.
[Automated review]
0a9a14c to
c123b0a
Compare
There was a problem hiding this comment.
Approval of this change is waiting on a maintainer rather than on code changes: the new indicator prop on DropdownMenuRadioGroup (and so on ContextMenuRadioGroup and BreadcrumbMenuRadioGroup) still has no current governing spec, because the engineering-owner decision is recorded only in the DropdownMenu record, which stays a draft. A maintainer needs to commit that decision as current (the Radio mark concept, FR15, the FR2 and anatomy amendments, and DEC-8) or point to an existing current record that already covers the prop. The implementation matches its description and leaves the default radio rendering unchanged. Once the decision is current, the "Radio indicator" anatomy entry in DropdownMenu.doc.mjs should also describe the check glyph that carries dropdown-menu-radio in check mode.
[Automated review]
There was a problem hiding this comment.
Owner ruling, cixzhang: this doesn't match Selector's existing prop, so the spec needs updating before it can be approved.
A single-choice menu and Selector both mark one chosen option, but they would describe that mark differently:
- Selector takes
indicatorPosition?: 'start' | 'end'. It chooses where the selection mark sits. The mark itself is always the theme'scheckindicator, which a theme can replace; no prop picks it. - This PR adds
indicator?: 'radio' | 'check'to the radio group. It chooses which mark is drawn and fixes where it goes (radio at the start, check at the end), under a different name and on a different axis.
A product picking between a Selector and a radio-group menu would meet two vocabularies for the same thing.
What the spec update should settle, in component:DropdownMenu, replacing the proposed Radio mark concept, FR15 and DEC-8:
- If the need is the mark's position, use Selector's
indicatorPosition: 'start' | 'end'with the same meaning. - If the need is a check mark instead of a radio circle, say why the theme's
radioindicator replacement can't express it. Selector's mark comes from the theme, not from a prop.
Code changes can follow once the spec settles that.
c123b0a to
dbc86bb
Compare
bf4c3ed to
dc76473
Compare
There was a problem hiding this comment.
Approval is waiting on a maintainer decision: the new indicatorPosition prop on DropdownMenuRadioGroup (and so on ContextMenuRadioGroup and BreadcrumbMenuRadioGroup) follows the Selector vocabulary the record owner asked for, but the DropdownMenu spec that would govern it is still a draft, so a maintainer needs to commit the proposed Mark position concept, FR18 and DEC-11 as current (or point to a current spec that covers the prop) and then re-review or dismiss the open change request. When recording it, they should settle whether unset stays pointer-dependent instead of Selector's end, whether a row whose mark draws nothing reserves no space (with the documented check recipe at start, only the chosen row's label is indented), and whether checkbox rows keep today's placement, which puts checkbox and radio marks on opposite edges of a mixed menu. In Chromium the change does what it describes under a mouse, under touch and in right-to-left, and menus without the prop render as before; the pr-a11y failure comes from a newer step on main and clears with a rebase.
[Automated review]
dc76473 to
9a0d1b1
Compare
There was a problem hiding this comment.
Approval is waiting on a maintainer decision: the new indicatorPosition prop on DropdownMenuRadioGroup (and so on ContextMenuRadioGroup and BreadcrumbMenuRadioGroup) follows the Selector vocabulary the record owner asked for, but the DropdownMenu spec that would govern it is still a draft, so a maintainer needs to commit the proposed Mark position concept, FR18 and DEC-11 as current (or point to a current spec that covers the prop) and then re-review or dismiss the open change request. When recording it, they should settle whether unset stays pointer-dependent instead of Selector's end, whether a row whose mark draws nothing reserves no space (with the documented check recipe at start, only the chosen row's label is indented), and whether checkbox rows keep today's placement, which puts checkbox and radio marks on opposite edges of a mixed menu. In Chromium the change does what it describes under a mouse, under touch and in right-to-left, menus without the prop render as before, and CI is green on this head, so nothing else is needed from you until that decision lands.
[Automated review]
9a0d1b1 to
5181913
Compare
|
Two older review threads, addressed earlier on this branch (now 1acf14e after a rebase):
Resolving both. The two non-blocking points are answered in their threads (5181913). |
There was a problem hiding this comment.
Approval is waiting on a maintainer decision: the new indicatorPosition prop on DropdownMenuRadioGroup (and so on ContextMenuRadioGroup and BreadcrumbMenuRadioGroup) follows the Selector vocabulary the record owner asked for, but the DropdownMenu spec that would govern it is still a draft, so a maintainer needs to commit the proposed Mark position concept, FR18 and DEC-11 as current (or point to a current spec that covers the prop) and then re-review or dismiss the open change request. When recording it, they should settle whether an unset group keeps its pointer-dependent edge instead of Selector's end, accept or reject FR18's new rule that a mark that draws nothing takes no room (so with the check recipe at start only the chosen row's label is indented), and say whether checkbox rows keep today's placement. In Chromium the change does what it describes under a mouse, under touch and in right-to-left, the new touch case closes the earlier test gap, menus without the prop render as before, and CI is green, so nothing else is needed from you until that decision lands.
[Automated review]
There was a problem hiding this comment.
Approval is waiting on a maintainer decision: the new indicatorPosition prop on DropdownMenuRadioGroup (and so on ContextMenuRadioGroup and BreadcrumbMenuRadioGroup) follows the Selector vocabulary the record owner asked for, but the DropdownMenu spec that would govern it is still a draft, so a maintainer needs to commit the proposed Mark position concept, FR18 and DEC-11 as current (or point to a current spec that covers the prop) and then re-review or dismiss the open change request. When recording it, they should settle whether an unset group keeps its pointer-dependent edge instead of Selector's end, whether FR18's rule that a mark that draws nothing takes no room stands (with the check recipe at start, only the chosen row's label is indented), and whether checkbox rows keep today's placement. The Mark position row now reads proposed, and in Chromium the change does what it describes under a mouse, under touch and in right-to-left while menus without the prop render as before, so nothing else is needed from you until that decision lands.
[Automated review]
f4ed94a to
47f04b2
Compare
There was a problem hiding this comment.
Approval is waiting on a maintainer decision: the new indicatorPosition prop on DropdownMenuRadioGroup (and so on ContextMenuRadioGroup and BreadcrumbMenuRadioGroup) follows the Selector vocabulary the record owner asked for, but both records that would govern it are still drafts, so a maintainer needs to commit the proposed Mark position concept, FR18 and DEC-11 in component:DropdownMenu and FR10 in component:DropdownMenuRadioGroup as current (or point to a current spec that covers the prop) and then re-review or dismiss the open change request. When recording it, they should settle whether an unset group keeps its pointer-dependent edge instead of Selector's end, whether FR18's rule that a mark that draws nothing takes no room stands (with the check recipe at start, only the chosen row's label is indented), and whether checkbox rows keep today's placement. In Chromium the change does what it describes under a mouse, under touch and in right-to-left, menus without the prop render as before, and nothing else is needed from you until that decision lands.
[Automated review]
47f04b2 to
e5846f1
Compare
There was a problem hiding this comment.
Approval is waiting on an engineering owner's spec decision, not on code changes: indicatorPosition on DropdownMenuRadioGroup (and so on ContextMenuRadioGroup and BreadcrumbMenuRadioGroup) rests on proposed rows in three draft records, namely the Mark position concept, FR18 and DEC-11 in component:DropdownMenu, FR10 in component:DropdownMenuRadioGroup and the reworded FR6 in component:DropdownMenuRadioItem. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so if they are an engineering owner), by making those rows current, or by naming an existing current spec that covers the prop, and the open change request also needs its author to re-review or dismiss it. The decision should settle whether an unset group keeps its pointer-dependent edge instead of Selector's end, whether FR18's rule that a mark drawing nothing takes no room stands (with the check recipe at start, only the chosen row's label is indented), and whether checkbox rows keep today's placement. In Chromium the change does what it describes under a mouse, under touch and in right-to-left, menus without the prop render as before, and nothing else is needed from you until that decision lands.
[Automated review]
…, as Selector's does
`DropdownMenuRadioGroup` takes `indicatorPosition` (`'start' | 'end'`),
Selector's prop and `IndicatorPosition` type with the same meaning: the
logical edge of each row that carries the radio mark, on every pointer.
Unset, the mark keeps today's placement, at the start under a fine pointer
and at the end under a coarse one. The mark stays the theme's `radio`
indicator; a menu that wants a check instead is wrapped in a nested Theme
that extends the product theme with `indicators: {radio: CheckIndicator}`.
ContextMenuRadioGroup and BreadcrumbMenuRadioGroup, the same component under
other names, take the prop too.
The draft DropdownMenu record proposes the Mark position concept, FR18 and
DEC-11 for the owner. pr-a11y runs the radio mark position Chromium contract.
Rebased onto main after meta#7530 as one commit; review rounds are folded in
unchanged.
…er a coarse pointer, and FR18 states Selector's FR3 The Chromium start case ran under a fine pointer, where start and an unset group place the mark alike, so it passed with the start pin dropped. A case in a touch-emulating context (pointer: coarse) checks that an unset group puts the mark after the label and that start keeps every mark before it; with the start pin dropped it fails. FR18 said 'with Selector's meaning' without stating Selector's FR3: it now says that a mark that draws nothing (an unchosen row when the theme's radio is CheckIndicator) takes no room, so at start only the chosen row's label is indented, which the nested-theme case shows (the check is drawn on the chosen row only). Its verification row names both.
…s decision is approved The concept row said its stability was stable while DEC-11, which decides it, is still proposed; the record's other proposed concept, Sub-menu regions, says proposed. Record wording only; check:knowledge and prettier pass.
…the indicatorPosition it passes to its rows Main now has the component's own record, component:DropdownMenuRadioGroup (draft). This change adds indicatorPosition to the context the group gives its radio items, which that record lists as value, onChange and hasCloseOnSelect only. The record now names it: proposed FR10 (the group passes the edge to every item, which places its mark, per component:DropdownMenu FR18), a Mark position concept, the ownership and compatibility notes, the edge in the item's indicator paint, a verification row and the Chromium spec in verified_by. The rule for where the mark sits stays component:DropdownMenu FR18 and DEC-11, which own the radio item.
… edge Main added component:DropdownMenuRadioItem, whose FR6, ownership and Coarse pointer state place the row's marker at the inline start on a fine pointer and the inline end on a coarse one, with no exception. This pull request's indicatorPosition pins it to one edge, so they now name the exception: unless proposed component:DropdownMenu FR18 places it. The record defers that edge to component:DropdownMenu, which owns the dropdown-menu-radio target the mark carries and FR18 itself. The FR6 cell stays within its column. Record only.
e5846f1 to
9fa93a8
Compare
There was a problem hiding this comment.
Approval is waiting on an engineering owner's spec decision, not on code changes: indicatorPosition on DropdownMenuRadioGroup (and so on ContextMenuRadioGroup and BreadcrumbMenuRadioGroup) rests on proposed rows in three draft records, namely the Mark position concept, FR18 and DEC-11 in component:DropdownMenu, FR10 in component:DropdownMenuRadioGroup and the reworded FR6 in component:DropdownMenuRadioItem. An engineering owner can record that decision in one of three ways: comment /approve-spec <full head SHA> on this pull request (the author may do so if they are an engineering owner), make those rows current, or name an existing current spec that covers the prop; the open change request also needs its author to re-review or dismiss it. In Chromium the change does what it describes under a mouse, under touch and in right-to-left, and menus without the prop render as before.
The decision should settle:
- whether an unset group keeps its pointer-dependent edge instead of Selector's
enddefault; - whether a mark that draws nothing takes no room, so that with the check recipe at
startonly the chosen row's label is indented; - whether checkbox rows keep today's edge.
[Automated review]
|
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. Some of our menus set a single option, such as how many rows to show, and should mark the choice at the row's end, as
Selectordoes, butDropdownMenuRadioItemputs its mark at the start under a mouse and no prop picks the edge.Why: to match, callers swap in a
DropdownMenuItemand write its radio role through a ref, which loses the radio item's behaviour.Problem and solution fit
DropdownMenuRadioGrouptakesSelector'sindicatorPosition: 'start' | 'end'on every pointer. Unset keeps today's placement (start under a mouse, end under a finger). The mark stays the theme'sradioindicator; a nestedThemecan map it to a check for one menu.Current specification
component:DropdownMenu(draft): this PR proposes a Mark position concept, FR18 and DEC-11;component:DropdownMenuRadioGroupnames the edge (proposed FR10) andcomponent:DropdownMenuRadioItemFR6 defers to FR18. It reusescomponent:Selector'sIndicatorPositiontype.Public delta and caller burden
DropdownMenuRadioGroup(alsoContextMenuRadioGroup,BreadcrumbMenuRadioGroup); nothing changes without it.Design and alternatives
radioin the root theme. No new prop, but it changes every menu in the app at once.roleonDropdownMenuItem. Works today, but callers rebuild the radio behaviour.indicatorPosition, as onSelector(chosen). One vocabulary for both: the position is a prop, the mark is the theme's. The cost is one prop on the group.Evidence
Unit cases check that every row stays a
menuitemradiowith the radio mark in either position and that a nested theme changes one menu only; onmainthe prop leaks to the DOM. A Chromium test checks both edges and keyboard selection; theendcase fails onmain.Writing rubric results
R1 narration: the concept, FR18, FR10 and DEC-11 are present tense.
R2 research: none.
R3 evidence: FR18's verification row names the tests.
R4 rejected alternatives: DEC-11 lists three.
R5 change references: none.
R6 open questions: none.
Scope
Testing
In Storybook, open Core/DropdownMenu → Radio Group Mark At End and open "Rows shown".