Repository navigation
docs(spec): SelectorOption records its shipped contract as current - #9144
Conversation
|
AI review status for this pull request.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:spec-owner-review
![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 an engineering owner's decision on the new component:SelectorOption record and the component:Selector ownership bullet, plus two record fixes. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner.
- FR1, AR1 and the
iconconcept row say every icon renders small, secondary and decorative, butSelectorOptionpasses a ReactNodeiconthroughrenderIconSlotunchanged (the test FR1 cites passes one), so please narrow them toIconTypeicons and list that evidence gap in the FR1 verification row. - The Design relationships rows cite
component:Itemfor requirements Item's record does not state, so cite this record's own owner direction or mark them unsettled.
[Automated review]
|
|
||
| | ID | Invariant | Basis | | ||
| | --- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------- | | ||
| | FR1 | SelectorOption renders one `component:Item` row: `icon` as a small secondary icon in the start slot when present, `label`, `description` when present, and `endContent` when present. Selector's default option renderer draws each option through SelectorOption too. | Current source and tests | |
There was a problem hiding this comment.
renderIconSlot makes only an IconType value (name, component or render object) a small secondary Icon. A ReactNode icon is returned unchanged, as the cited renderValue test shows with <span data-testid="lock-glyph" />. Suggest: "icon in the start slot when present: an IconType icon as a small secondary Icon, a ReactNode as given". Update the concept row at L78 the same way.
| ## Accessibility contract | ||
|
|
||
| - **AR1 — Content only.** SelectorOption adds no role, name, or state; the | ||
| Selector option row it sits in owns them. The icon is decorative. |
There was a problem hiding this comment.
Only an IconType icon is decorative (Icon's aria-hidden default). A ReactNode icon keeps the semantics its caller gives it, so this should say that, and the FR1 verification row should list the missing icon-presentation test as a gap.
| | Row | Lays out the option content like a built-in option. | `component:Item` | Supporting | FR1, FR3 | | ||
| | Icon | Identifies the option at a glance. | `component:Item` | Supporting | FR1 | | ||
| | Label | Names the option. | `component:Item` | Prominent | FR1, FR3 | | ||
| | Description | Adds secondary detail. | `component:Item` | Supporting | FR1, FR2 | | ||
| | End content | Holds caller content after the text. | `component:Item` | Supporting | FR1 | |
There was a problem hiding this comment.
component:Item's design table states only that start and end content annotate the label's first line, and that the label's first line is the reference line. These rows' requirements are this record's own; cite its owner direction or mark them unsettled.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the new component:SelectorOption record and the component:Selector ownership bullet. An engineering owner records it by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Two record fixes are needed either way:
- The non-goal "A
refor other DOM props; SelectorOption accepts neither" contradictsarchitecture:public-component-apiINV8, so please record it as a requirement with an implementation gap, as other component records do, or have the architecture record amended. IconTypecovers only icon components, butSelectorOptionalso draws semantic icon names such asicon="search"as Icon, so FR1, AR1 and theiconconcept row should say that semantic names and icon components render through Icon, while other ReactNode content renders as given.
[Automated review]
| owned by `component:Selector`. | ||
| - The trigger's sizing around a rendered value — owned by `component:Selector`. | ||
| - Item's layout, density, and targets — owned by `component:Item`. | ||
| - A `ref` or other DOM props; SelectorOption accepts neither. |
There was a problem hiding this comment.
architecture:public-component-api INV8 says a public DOM component accepts ref and connects it to its contract element. SelectorOption is public and renders an Item <div>, so listing this as a non-goal turns the shipped gap into stated direction. Please record it as a requirement with an implementation gap (as Divider.spec.md FR6 and Citation.spec.md FR5 do), or have INV8 amended with an exception.
|
|
||
| | ID | Invariant | Basis | | ||
| | --- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------- | | ||
| | FR1 | SelectorOption renders one `component:Item` row: `icon` in the start slot when present (an `IconType` icon as a small secondary Icon, a ReactNode as given), `label`, `description` when present, and `endContent` when present. Selector's default option renderer draws each option through SelectorOption too. | Current source and tests | |
There was a problem hiding this comment.
IconType is only the component type, so a semantic name such as icon="search" falls under "a ReactNode as given" here. But renderIconSlot draws every string as a small, secondary, aria-hidden Icon, and an unregistered name renders nothing. component:ComplexSelector AV2's wording fits: semantic icon names and icon components render through Icon; other ReactNode content renders as given. The same applies to L78 and AR1.
| | --- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------- | | ||
| | FR1 | SelectorOption renders one `component:Item` row: `icon` in the start slot when present (an `IconType` icon as a small secondary Icon, a ReactNode as given), `label`, `description` when present, and `endContent` when present. Selector's default option renderer draws each option through SelectorOption too. | Current source and tests | | ||
| | FR2 | The row uses the caller's `layout` (`stacked` by default). When its host pins the row height (a Selector trigger inside an InputGroup), the row is `inline` whatever the caller passed. | Current source and tests | | ||
| | FR3 | The row carries the `selector-option` target. Its own block and inline padding and corner radius are zero and its minimum width is zero, so a long label ellipsizes inside a narrow host. Caller `xstyle` merges after these base styles; `className` and `style` reach the row. | `architecture:component-theming-surface`; current source | |
There was a problem hiding this comment.
Item truncates only string labels (typeof label === 'string'), so "a long string label ellipsizes" is the shipped behaviour; a ReactNode label wraps.
bde51be to
d4ba1ae
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the new component:SelectorOption record and the component:Selector ownership bullet; an engineering owner records it by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. The earlier ref and icon fixes are in. One wording fix is still needed either way:
- The
layoutconcept row (L76) and the Review checks row (L34) say SelectorOption folds inline anywhere inside an InputGroup, but only a Selector trigger value does (FR2), and the options in that Selector's list keep the caller's layout, so please scope both rows to the trigger value. - The End-user impact row (L31) should say a long string label ellipsizes, as FR3 does.
[Automated review]
| | End-user impact | People see custom Selector options and the selected value in the same row shape as built-in options, and a long label ellipsizes instead of overflowing a narrow popover. | | ||
| | Builder impact | Builders return it from `renderOption` or `renderValue` to keep the `selector-option` target and the shared row layout while choosing the content. | | ||
| | Compatibility/readiness | Records shipped behavior; no API, default, or DOM change. The missing `ref` and DOM props are listed under Current-state impact. | | ||
| | Review checks | Reject a dropped icon, description, or end content, a layout that ignores the caller outside an InputGroup or stays stacked inside one, or a lost `selector-option` target. | |
There was a problem hiding this comment.
Only a Selector trigger value folds inside an InputGroup (FR2; Selector.tsx provides the row-layout context around renderValue alone). The options in a grouped Selector's list keep the caller's layout, so "stays stacked inside one" would reject correct code. Suggest: "…or stays stacked as a Selector trigger value inside one".
|
|
||
| | Concept | Closed values or states | Meaning | Availability by variant/orientation | | ||
| | ----------- | ----------------------- | ------------------------------------------------------------------------------------------------------------------------------------ | -------------------------------------------------------- | | ||
| | layout | `stacked`, `inline` | `stacked` puts the description on its own line; `inline` keeps label and description on one line. | Caller's choice, except inside an InputGroup (`inline`). | |
There was a problem hiding this comment.
This widens FR2: list options of a Selector inside an InputGroup, and a SelectorOption placed directly in one, stay the caller's layout. Suggest: "Caller's choice, except as a Selector trigger value inside an InputGroup (inline)."
| | ----------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | Public contract | `SelectorOption` is exported beside Selector and takes `label` (required), `icon`, `description`, `layout` (`stacked` by default, or `inline`), `endContent`, `xstyle`, `className`, and `style`. | | ||
| | Behavior | Renders one Item row of an optional icon, the label, an optional description, and optional end content, with the row's own padding and corner radius removed so its host row lays it out. | | ||
| | End-user impact | People see custom Selector options and the selected value in the same row shape as built-in options, and a long label ellipsizes instead of overflowing a narrow popover. | |
There was a problem hiding this comment.
Item truncates only string labels, so a ReactNode label wraps (as FR3 says). Suggest "a long string label ellipsizes".
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the new component:SelectorOption record and the component:Selector ownership bullet, which state requirements the current specs do not yet hold; an engineering owner records it by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. The earlier wording fixes for the InputGroup fold and the string-label ellipsis are in, and the record now matches the shipped SelectorOption source and its cited tests, so no other change is needed.
[Automated review]
|
/approve-spec 117a4ff |
github-actions
Bot
removed
the
needs:spec-owner-review
Exact decision needed
Add
component:SelectorOptionas a current record (it has its own.doc.mjs), recording its shipped behavior, and add one "Does not own" line to the currentcomponent:Selectorrecord pointing at it. No behavior, API, default, or DOM change.Existing authority searched
pnpm specs:for packages/core/src/Selector/SelectorOption.tsx→ current records that govern it:component:Selector(which maps theselector-optiontarget as "SelectorOption-rendered content"),component:Item,architecture:component-theming-surface, andarchitecture:public-component-api. Nocomponent:SelectorOptionrecord existed.Selector.spec.md: fix(Selector, MultiSelector): a closed list opens on the selected option, never a disabled one #8868 and fix(Selector, MultiSelector): a query change no longer keeps the highlight from the previous list #8871 rebase on top of this record; this PR adds one bullet to Selector's "Does not own" list and changes nothing else in it. refactor(Selector): the presentation controller holds its choice through the layer runtime's presentation hold #8591, feat(core): in Chrome on Android a Tall BottomSheet rides Gboard's slides #8566, and fix(InputGroup): an error status on the group marks its members invalid #9030 touch Selector source only.Decision
SelectorOption.spec.md(new,approved_by: cixzhang,approved_at: 2026-11-01): the Item row it renders, the host-pinnedinlinelayout inside an InputGroup, theselector-optiontarget and row reset, and root styling.Selector.spec.md: one "Does not own" bullet namingcomponent:SelectorOption. No requirement, decision, or map in Selector's record changes.Rejected alternatives
None.
Claim scope and non-goals
Shipped behavior only, records live under
docs/specs/components/Selector/; traced fromSelectorOption.tsx,SelectorRowLayoutContext.ts,Selector.tsx,SelectorOption.doc.mjs, andSelector.test.tsx. No code is fixed.Evidence and implementation relationship
Selector.test.tsxend-content, render-value, layout-fold, and narrow-list cases; each verification row lists its evidence gaps.Current-state impact:
refnor other DOM props, againstarchitecture:public-component-apiINV5, INV8.Writing rubric results
Scope
Validation
pnpm check:knowledge: alignedgit diff --check: clean