feat(selector): open the menu below by default - #4976
Conversation
The selected-item overlay — the open menu pulled up so the selected option sits over the trigger, native-select style — is no longer the default. Selector now opens below with the standard --spacing-1 clearance, the same recipe DropdownMenu, MultiSelector, and ComplexSelector already use; search mode gains the clearance too (it used to sit flush). The overlay survives as opt-in hasSelectedItemOverlay, and only engages on below placement — an explicit above/start/end still wins, as before. Why the flip, measured rather than assumed: - The trigger keeps DOM focus while the popup is open and the overlay covers it entirely — SC 2.4.11 (AA) focus-obscured territory. - The overlay quietly degenerates: with the selected option past the listbox fold the menu pins to the viewport top (the measured margin equals triggerRect.bottom), and the no-value placeholder state that every form starts in still pulls the menu over its own trigger. - It was already only half the default: hasSearch switches to plain below, MultiSelector hard-codes below, and every other menu-shaped popup in core opens below. placement was also undocumented everywhere that matters — absent from Selector.doc.mjs, so missing from the docsite props table and from astryx component Selector --dense. Both it (@default 'below') and the new prop are documented now. Mechanically small: overlay mode was already below placement plus a measured negative margin-block-start, so the flip removes the margin and adds the offset; useSelectedItemOffset is untouched and stays public API. The clearance is gated off in overlay mode — offset sets both block edges (flip-safe per facebook#4803) while the overlay margin is inline, and mixing them would offset the far edge asymmetrically. Verified: 3 new tests red against the untouched tree, then green; 113 pass in Selector.test.tsx; full suite 9887 pass with only the two pre-existing macOS cli-casing failures from the recorded baseline; docsite 372 pass; core tsc, docsite and storybook typecheck, CI=true lint, check:repo (changeset included), and prettier all clean. Storybook screenshots confirm both modes. Changeset: [breaking] minor per the 0.x convention. To restore the old look on an instance, add hasSelectedItemOverlay. Refs facebook#4227
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR Modified ComponentsSelector (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
|
On the naming call (#2 in your list): rather than a new boolean, I'd rather we expand placement?: 'above' | 'below' | 'start' | 'end' | 'overlay' | 'offset';
Two reasons I prefer this to Separately, I'm not sure yet that we want to change the default at all — that's a bigger call than the naming, and I'd like to keep it separate from the API shape. Would you split this into the placement API (values + docs, no behavior change) and the default flip? The first can land immediately. Also worth knowing: #5018 touches |
cixzhang
left a comment
There was a problem hiding this comment.
Thanks for this — your analysis on the issue did most of the work, and the placeholder case is the sharpest version of it: before anyone has picked anything, the trigger is completely hidden behind its own menu.
Following up on my comment above, plus one thing that changed since you opened this: the --spacing-1 clearance landed on main as #5003, so it comes out of your diff and takes most of the conflict with it.
That leaves the placement API half quite small — could you send just that first?
'overlay'and'offset'onLayerPlacement, behavior unchanged ·useLayer.tsx:72placementin the props table, still missing on main ·Selector.doc.mjs:85
The flip, the changeset and the overlay story then ride on a second PR, where the default is the only thing left to decide.
If you'd rather talk it through with someone, we're in Discord.
[Reviewed by Robohands]
| * hasSearch uses standard layer positioning instead. | ||
| * @default false | ||
| */ | ||
| hasSelectedItemOverlay?: boolean; |
There was a problem hiding this comment.
Superseded by the 'overlay' / 'offset' placement values. Comes out with the flip.
| '@astryxdesign/core': minor | ||
| --- | ||
|
|
||
| [breaking] Selector menus open below the trigger by default (#4227). The old native-select behavior — the open menu overlaying the trigger with the selected option pinned over it — is now opt-in via `hasSelectedItemOverlay`. `placement` keeps working as before and now documents its `'below'` default; Selector menus also gain the standard `--spacing-1` clearance that DropdownMenu, MultiSelector, and ComplexSelector already use (search mode included, which used to sit flush). |
There was a problem hiding this comment.
Search mode isn't flush any more, #5003 landed that. Needs a reword.
|
Checking in on Selector placement: the current review asks to split the small placement API/docs change from the default-behavior decision. Would you like to continue, have us carry it, or close this for now? |
Closes #4227
What this does
Selector's menu now opens underneath the button, with the same small gap every other Astryx menu uses. The old behavior — the menu covering the button with the selected option pinned on top — is still available per instance via a new
hasSelectedItemOverlayprop.Why
Following up my analysis on the issue: the overlay default covered the very button that keeps keyboard focus (SC 2.4.11 territory), quietly fell apart once the selected option sat past the listbox fold (the menu just pinned to the top of the screen — including the empty placeholder state every form starts in), and made default Selector the odd one out: DropdownMenu, MultiSelector, ComplexSelector, and Selector-with-search all open below already.
The three calls left open on the issue — proposed here, easy to veto piecemeal
hasSelectedItemOverlay— thehas*convention, anchored to "selected item" (the code's own term), since bare "overlay" already means theOverlaycomponent and its tokens.--spacing-1, the standard clearance DropdownMenu, MultiSelector, and ComplexSelector already use. Search-mode Selector gains it too (it used to sit flush).What changed
hasSelectedItemOverlayrestores the overlay per instance (only on below placement — explicitabove/start/endstill wins, like today).placementis now documented — it was missing from the docsite props table andastryx component Selector --denseentirely (the "(b)" half of the issue, worth having either way).[breaking]→ minor: every default Selector changes visually.How to see it
Storybook → Core/Selector: Default now opens below with the gap; Selected Item Overlay shows the opt-in.
Test plan
main, green after: default = below +--spacing-1on both block edges (flip-safe per [Bug] ComplexSelector popup has no offset when placement="above" #4803), explicit placements get the clearance, search mode gets it and never overlays.hasSelectedItemOverlayand still pass — the measurement path is untouched.packages/cliare pre-existing and unrelated); docsite 372 passing;pnpm lint:strict,pnpm test,pnpm buildall green.