react-combobox: add separate Dropdown component#23550
react-combobox: add separate Dropdown component#23550smhigley merged 24 commits intomicrosoft:masterfrom
Conversation
📊 Bundle size report
🤖 This report was generated against 118f7ddaac0f7e1f705f5fb93af7722a35a27610 |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 1cffa6c:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 118f7ddaac0f7e1f705f5fb93af7722a35a27610 (build) |
| const [triggerWithPopup, listboxWithPopup] = useComboboxPopup(props, triggerShorthand, listboxShorthand); | ||
| const [triggerSlot, listboxSlot] = useTriggerListboxSlots(props, baseState, ref, triggerWithPopup, listboxWithPopup); |
There was a problem hiding this comment.
As I mentioned in the other PR: probably these hooks should accept and return state to be consistent with other components:
const state = {/* ... */}
useComboboxPopup(state)
useComboboxTrigger(state)
return stateThere was a problem hiding this comment.
I actually tried doing this initially, but the problem is essentially with typing. The issue is that Combobox vs. Dropdown have different and incompatible primary slot types (button vs. input, with different children types), and I didn't find a way to type the state prop and return type in a way that would work without a bunch of explicit as X casting.
The other reason is that I made Combobox's primary slot be input, and Dropdown's primary slot be button. This is mostly a stylistic opinion, but it ended up seeming cleaner to me to take/return the slot props themselves rather than have the hook check for e.g. if('input' in state) etc.
I'd be happy to work with you on that more, if you have any thoughts on how to make that work. For now, both hooks are fully internal and not exported, so we can change this as much as we want without affecting the public API :)
Would you mind to add a bundle size fixture for |
| ...shorthands.borderRadius(tokens.borderRadiusMedium), | ||
| boxSizing: 'border-box', | ||
| display: 'inline-block', | ||
| columnGap: tokens.spacingHorizontalXXS, |
packages/react-components/react-combobox/src/components/Dropdown/Dropdown.types.ts
Outdated
Show resolved
Hide resolved
packages/react-components/react-combobox/src/stories/Dropdown.stories.tsx
Outdated
Show resolved
Hide resolved
packages/react-components/react-combobox/src/stories/DropdownDefault.stories.tsx
Outdated
Show resolved
Hide resolved
| * Callback when the open/closed state of the dropdown changes | ||
| */ | ||
| onOpenChange?: (e: ComboboxOpenEvents, data: ComboboxOpenChangeData) => void; | ||
| onOpenChange?: (e: ComboboxBaseOpenEvents, data: ComboboxBaseOpenChangeData) => void; |
There was a problem hiding this comment.
Nit: Not a fan of having Base in the type names.
There was a problem hiding this comment.
I know, I just can't think of anything else 😅
This adds a
Dropdowncomponent to thereact-comboboxpackage, and updatesComboboxto be an editable field with an<input>as the primary slot.This PR depends on the changes in #23499
Note: there are some quirks with how the
inputbehaves when selecting vs. typing that will be fixed in a separate PR.