react-combobox: move Combobox functionality to shared hooks#23499
react-combobox: move Combobox functionality to shared hooks#23499smhigley merged 14 commits intomicrosoft:masterfrom
Conversation
📊 Bundle size report
🤖 This report was generated against 675acea49c97f10837ddee9b8c4350ca27750125 |
|
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 99e8f58:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: c446a15764655758650fc5ff865652abe8d9fa44 (build) |
139c9da to
d30e1c2
Compare
packages/react-components/react-combobox/src/ComboboxBase/ComboboxBase.types.ts
Show resolved
Hide resolved
packages/react-components/react-combobox/src/utils/useComboboxPopup.ts
Outdated
Show resolved
Hide resolved
e57d298 to
99e8f58
Compare
| ref: React.Ref<HTMLButtonElement | HTMLInputElement>, | ||
| triggerSlot?: ExtractSlotProps<Slot<'input'>> | ExtractSlotProps<Slot<'button'>>, | ||
| listboxSlot?: ExtractSlotProps<Slot<typeof Listbox>>, | ||
| ): [ExtractSlotProps<Slot<'input'>> | ExtractSlotProps<Slot<'button'>>, ExtractSlotProps<Slot<typeof Listbox>>] { |
There was a problem hiding this comment.
I could be wrong, but is not our pattern to return objects? (all other our hooks doing that except utility ones that mimic React's behavior)
| // resolve listbox shorthand props | ||
| const listbox: typeof listboxSlot = { | ||
| multiselect, | ||
| tabIndex: undefined, |
There was a problem hiding this comment.
Just wondering why tabIndex is required there as it overrides nothing...
| setOpen(event, false); | ||
| break; | ||
| case 'CloseSelect': | ||
| !multiselect && setOpen(event, false); |
There was a problem hiding this comment.
nit: Can we prefer if conditions? It will not make code longer, but more readable 👍
if (!multiselect) {
setOpen(event, false);
}| import type { ComboboxBaseProps } from './ComboboxBase.types'; | ||
| import { Listbox } from '../components/Listbox/Listbox'; | ||
|
|
||
| export function useComboboxPopup( |
There was a problem hiding this comment.
nit: Naming is hard, but should not it be a popover to be consistent with Popover component? Or is it intentional?
| return [ | ||
| { ...triggerShorthand, ref: useMergedRefs(triggerShorthand?.ref, targetRef) }, | ||
| { ...listboxShorthand, ref: useMergedRefs(listboxShorthand?.ref, containerRef) }, | ||
| ]; |
There was a problem hiding this comment.
Other our hooks (use*Styles and etc.) are mutating the state instead of creating new object. Is it intentional there?
const { targetRef, containerRef } = usePositioning(popperOptions);
triggerShorthand.ref = useMergedRefs(triggerShorthand?.ref, targetRef)| triggerShorthand?: ExtractSlotProps<Slot<'button'>>, | ||
| listboxShorthand?: ExtractSlotProps<Slot<typeof Listbox>>, |
There was a problem hiding this comment.
nit: We use term "shorthand" to unresolved slot props i.e. a union of string | number | SlotProps. Is there sense to rename it to triggerSlotProps & listboxSlotProps to make it clearer? (it will be a clear indicator that a shorthand was resolved to props already)
| * Render the combobox's popup inline in the DOM. | ||
| * This has accessibility benefits, particularly for touch screen readers. | ||
| */ | ||
| inlinePopup?: boolean; |
There was a problem hiding this comment.
Curiosity: why it was renamed?
This PR splits functionality in Combobox that will be shared with Dropdown into a folder of shared hooks. It depends on the changes in #23492 before it can be merged.
You can see where this fits into the larger combobox work (with some differences) in #22906
I'm very indifferent about the folder name "ComboboxBase", but I haven't really thought of a better one. I'm also happy to put the files in
utils, but since they're so specific to the component slots and state, it made sense to me to have them in a separate folder.