[select] Add data-popup-side to trigger#4671
Conversation
commit: |
Bundle size
PerformanceTotal duration: 1,502.04 ms 🔺+216.34 ms(+16.8%) | Renders: 53 (+0) | Paint: 2,328.30 ms 🔺+343.77 ms(+17.3%)
...and 7 more. View full report Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
flaviendelangle
left a comment
There was a problem hiding this comment.
PR Review Summary — #4671
[select] Add data-popup-side to trigger by @mj12albert · 8 files, +70 / −16 · Closes #4667
Three agents reviewed the diff against the upstream mui/base-ui repo (code-reviewer, pr-test-analyzer, silent-failure-hunter). Bottom line: ship it, with one design question worth confirming and a small follow-up test.
Critical Issues
None.
Important Issues
1. positioning.side vs renderedSide divergence — design question, not a bug
packages/react/src/select/positioner/SelectPositioner.tsx:139-142
Select's positioner derives renderedSide = alignItemWithTriggerActive ? 'none' : positioning.side and uses it for SelectPositionerState.side. The new effect pushes the raw positioning.side to the store. So under the default alignItemWithTrigger={true}, the trigger will expose data-popup-side="bottom" (or whichever side floating-ui resolved) while the positioner itself reports data-side="none".
- The code-reviewer flagged this as an inconsistency.
- The silent-failure-hunter argues this is intentionally correct:
data-popup-sideshould reflect actual placement, not the rendered-as-noneoverride.
Both views are defensible. Worth a one-line note in the PR confirming the intent — and an explicit test asserting the contract under default alignItemWithTrigger.
2. Test gaps
packages/react/src/select/trigger/SelectTrigger.test.tsx:178-203
Only side="right" with alignItemWithTrigger={false} is covered. Worth adding (and ideally for Combobox too, for parity):
- Default
alignItemWithTriggermode — the common case, currently untested. Settles the question above. - Collision-flip — requested
side="bottom"but viewport forces flip totop. Most failure-prone path; protects against a refactor that ever read the requested prop instead of the resolved side.
defaultOpen initial-render gating is a nice-to-have but lower value.
Suggested collision-flip test:
it.skipIf(isJSDOM)('reflects the resolved side after collision flip', async () => {
const { user } = await render(
<div style={{ position: 'fixed', top: 0, left: 0, height: '100vh', width: '100vw' }}>
<div style={{ position: 'absolute', bottom: 4, left: 100 }}>
<Select.Root>
<Select.Trigger data-testid="trigger">Trigger</Select.Trigger>
<Select.Portal>
<Select.Positioner side="bottom" alignItemWithTrigger={false}>
<Select.Popup style={{ height: 200 }}>
<Select.Item value="a">a</Select.Item>
</Select.Popup>
</Select.Positioner>
</Select.Portal>
</Select.Root>
</div>
</div>,
);
const trigger = screen.getByTestId('trigger');
await user.click(trigger);
await waitFor(() => expect(screen.queryByRole('listbox')).not.toBe(null));
expect(trigger).toHaveAttribute('data-popup-side', 'top');
});Suggestions
- Cleanup the store on positioner unmount (
packages/react/src/select/positioner/SelectPositioner.tsx:142-145). TheuseIsoLayoutEffecthas no cleanup, sostore.popupSidelingers across open/close cycles. It's currently safe because the trigger gates onmounted && positionerElement— butpositionerElementandlistElementare nulled on unmount, so leavingpopupSidestale is a foot-gun for future readers. A one-linerreturn () => store.set('popupSide', null);makes the lifecycle pattern self-consistent. - Shared mapping for parity with Combobox. Combobox extracts
triggerStateAttributesMappingintocombobox/utils/stateAttributesMapping.ts; Select inlines it inSelectTrigger.tsx:32-37. Extracting a shared helper would reduce future drift. Non-blocking.
Strengths
- Pattern faithfully mirrors Combobox: identical store field + selector, identical
useIsoLayoutEffectpush, identicalmounted && positionerElementgating, identical state-attribute mapping, identicalSide | nulltyping from the canonicaluseAnchorPositioningmodule. - No stale-attribute window on close. Render-time computation
popupSide = mounted && positionerElement ? popupSideValue : null(SelectTrigger.tsx:94) is synchronous with the positioner's ref-callbacksetPositionerElement(null), so the attribute disappears in the same render. - Conventions respected:
useIsoLayoutEffect, noas any, no shadow-DOM-unsafe globals, noflushMicrotasksin the test,it.skipIf(isJSDOM)correctly applied. - Docs kept in sync:
page.mdx,select/types.md, JSDoc onSelectTriggerState.popupSide, andSelectTriggerDataAttributes.tsall match — looks likepnpm docs:apiwas run. - No silent failures. The
nullfallback is a correct expression of "popup is closed", not error-hiding.getStateAttributesPropsproperly omits the attribute when the mapping returnsnull. - Initial state is
nullinSelectRoot.tsx:154— attribute correctly absent on first render.
Recommended Action
- Confirm with @mj12albert whether exposing
data-popup-side="bottom"whiledata-side="none"(under defaultalignItemWithTrigger) is intentional. - If yes, add a default-mode test plus a collision-flip test (consider mirroring on Combobox for parity).
- Optional: add the unmount cleanup one-liner.
No re-review needed after these — they're all small.
Add
data-popup-sideto trigger matching ComboboxCloses #4667