DRAFT: Combobox/Dropdown components using slots for trigger/listbox by smhigley · Pull Request #22906 · microsoft/fluentui

smhigley · 2022-05-09T21:31:08Z

Related to #22797, this shows a sample of how Combobox/Dropdown could recompose a ComboboxBase component with separate components in the input slot.

Important note: this PR is for discussion only. If we go this route, I'll break up the changes into multiple smaller PRs for detailed review.

There are some outstanding issues with how to type the input slot that apply beyond just this PR. Aside from those, this shows the slot approach, so an editable vs select-only combobox would be authored like this:

<Combobox {...props} appearance="outline" value='Ferret'>
     {options.map(option => (
         <Option key={option} disabled={option === 'Ferret'}>
           {option}
         </Option>
       ))}
</Combobox>

vs.

<Dropdown {...props} appearance="outline" value='Ferret'>
     {options.map(option => (
         <Option key={option} disabled={option === 'Ferret'}>
           {option}
         </Option>
       ))}
</Dropdown>

The alternative (trigger/listbox as children) would be to author editable vs. non-editable like this:

<Combobox {...props} value='Ferret'>
     <ComboButton appearance="outline" placeholder="Select an animal" />
     <Listbox>
       {options.map(option => (
         <Option key={option} disabled={option === 'Ferret'}>
           {option}
         </Option>
       ))}
     </Listbox>
</Combobox>

vs.

<Combobox {...props} value='Ferret'>
     <ComboboxInput appearance="outline" placeholder="Select an animal" />
     <Listbox>
       {options.map(option => (
         <Option key={option} disabled={option === 'Ferret'}>
           {option}
         </Option>
       ))}
     </Listbox>
</Combobox>

fabricteam · 2022-05-09T21:40:45Z

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
`react-combobox` Combobox	`61.494 kB` `21.019 kB`	`61.311 kB` `20.981 kB`	`-183 B` `-38 B`

_{🤖 This report was generated against 6807d5428ee2e508e63f88191d1d11112907dfd4}

size-auditor · 2022-05-09T21:47:26Z

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 845b826ec74c72d8d543c4271930d7e42c0b9c40 (build)

fabricteam · 2022-05-09T21:55:05Z

Perf Analysis (`@fluentui/react-northstar`)

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
AlertMinimalPerf.default	288	253	1.14:1
LabelMinimalPerf.default	396	362	1.09:1
ReactionMinimalPerf.default	389	356	1.09:1
ListWith60ListItems.default	705	651	1.08:1
AnimationMinimalPerf.default	588	550	1.07:1
AvatarMinimalPerf.default	191	178	1.07:1
PortalMinimalPerf.default	175	163	1.07:1
AccordionMinimalPerf.default	146	138	1.06:1
ButtonOverridesMissPerf.default	1576	1481	1.06:1
CardMinimalPerf.default	565	535	1.06:1
ListNestedPerf.default	619	586	1.06:1
ImageMinimalPerf.default	389	370	1.05:1
LoaderMinimalPerf.default	737	699	1.05:1
TableMinimalPerf.default	409	389	1.05:1
DialogMinimalPerf.default	775	748	1.04:1
IconMinimalPerf.default	616	593	1.04:1
ChatDuplicateMessagesPerf.default	299	289	1.03:1
HeaderSlotsPerf.default	771	749	1.03:1
MenuButtonMinimalPerf.default	1784	1724	1.03:1
ProviderMinimalPerf.default	411	398	1.03:1
CustomToolbarPrototype.default	2850	2769	1.03:1
TreeMinimalPerf.default	840	814	1.03:1
HeaderMinimalPerf.default	352	346	1.02:1
SplitButtonMinimalPerf.default	4457	4367	1.02:1
BoxMinimalPerf.default	348	345	1.01:1
ButtonMinimalPerf.default	165	163	1.01:1
DatepickerMinimalPerf.default	5922	5862	1.01:1
GridMinimalPerf.default	346	341	1.01:1
ListMinimalPerf.default	514	510	1.01:1
RosterPerf.default	1091	1078	1.01:1
RadioGroupMinimalPerf.default	445	440	1.01:1
RefMinimalPerf.default	234	231	1.01:1
SliderMinimalPerf.default	1726	1712	1.01:1
ProviderMergeThemesPerf.default	1262	1257	1:1
TableManyItemsPerf.default	1928	1922	1:1
TooltipMinimalPerf.default	1074	1077	1:1
CheckboxMinimalPerf.default	2681	2711	0.99:1
EmbedMinimalPerf.default	4103	4155	0.99:1
FlexMinimalPerf.default	268	272	0.99:1
ItemLayoutMinimalPerf.default	1158	1172	0.99:1
MenuMinimalPerf.default	838	845	0.99:1
SkeletonMinimalPerf.default	350	354	0.99:1
StatusMinimalPerf.default	660	669	0.99:1
AttachmentSlotsPerf.default	1062	1079	0.98:1
CarouselMinimalPerf.default	457	465	0.98:1
DropdownMinimalPerf.default	3055	3117	0.98:1
PopupMinimalPerf.default	625	639	0.98:1
VideoMinimalPerf.default	660	676	0.98:1
AttachmentMinimalPerf.default	143	148	0.97:1
FormMinimalPerf.default	395	406	0.97:1
InputMinimalPerf.default	1319	1357	0.97:1
LayoutMinimalPerf.default	344	354	0.97:1
TextAreaMinimalPerf.default	480	495	0.97:1
ButtonSlotsPerf.default	506	525	0.96:1
DividerMinimalPerf.default	344	357	0.96:1
DropdownManyItemsPerf.default	656	686	0.96:1
ListCommonPerf.default	616	642	0.96:1
TreeWith60ListItems.default	156	162	0.96:1
TextMinimalPerf.default	334	352	0.95:1
ToolbarMinimalPerf.default	947	992	0.95:1
ChatMinimalPerf.default	721	766	0.94:1
SegmentMinimalPerf.default	326	346	0.94:1
ChatWithPopoverPerf.default	361	397	0.91:1

fabricteam · 2022-05-09T22:03:19Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
BaseButton	mount	914	891	5000
Breadcrumb	mount	2782	2736	1000
Checkbox	mount	1530	1518	5000
CheckboxBase	mount	1270	1266	5000
ChoiceGroup	mount	4724	4791	5000
ComboBox	mount	970	1035	1000
CommandBar	mount	10597	10530	1000
ContextualMenu	mount	11609	11705	1000
DefaultButton	mount	1145	1145	5000
DetailsRow	mount	3891	3866	5000
DetailsRowFast	mount	3965	3828	5000
DetailsRowNoStyles	mount	3603	3631	5000
Dialog	mount	2286	2297	1000
DocumentCardTitle	mount	166	171	1000
Dropdown	mount	3309	3323	5000
FocusTrapZone	mount	1857	1793	5000
FocusZone	mount	1865	1845	5000
IconButton	mount	1781	1745	5000
Label	mount	340	350	5000
Layer	mount	2979	2918	5000
Link	mount	459	467	5000
MenuButton	mount	1494	1498	5000
MessageBar	mount	2156	2079	5000
Nav	mount	3350	3348	1000
OverflowSet	mount	1111	1110	5000
Panel	mount	2192	2163	1000
Persona	mount	1020	1039	1000
Pivot	mount	1425	1469	1000
PrimaryButton	mount	1316	1308	5000
Rating	mount	7724	7773	5000
SearchBox	mount	1323	1340	5000
Shimmer	mount	2466	2522	5000
Slider	mount	1935	1955	5000
SpinButton	mount	5063	5059	5000
Spinner	mount	431	442	5000
SplitButton	mount	3224	3229	5000
Stack	mount	528	535	5000
StackWithIntrinsicChildren	mount	2280	2294	5000
StackWithTextChildren	mount	5269	5206	5000
SwatchColorPicker	mount	11675	11811	5000
TagPicker	mount	2728	2738	5000
TeachingBubble	mount	93275	93867	5000
Text	mount	428	429	5000
TextField	mount	1404	1487	5000
ThemeProvider	mount	1235	1204	5000
ThemeProvider	virtual-rerender	632	649	5000
ThemeProvider	virtual-rerender-with-unmount	1896	1908	5000
Toggle	mount	804	815	5000
buttonNative	mount	133	127	5000

Hotell · 2022-05-10T12:30:51Z

tip: if your PR is in draft lets make it so

codesandbox-ci · 2022-06-09T23:57:01Z

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 3fe5f44:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

behowell

This looks really great! I really like the way that you're sharing the functionality between Combobox and Dropdown through hooks.

behowell · 2022-06-10T01:13:17Z

packages/react-components/react-combobox/src/components/Combobox/Combobox.test.tsx

-    primarySlot: 'trigger',
-    // don't test deprecated className export on new components
+    primarySlot: 'input',
+    // don't test derpecated className export on new components


I like the new word derpecated 😂, but I think it's a tyop here?

well now I like "derpecated" too 🤣

behowell · 2022-06-10T01:22:19Z

packages/react-components/react-combobox/src/ComboboxBase/useComboboxBaseSlots.ts

+    (state as DropdownState).button = {
+      'aria-expanded': open,
+      'aria-activedescendant': open ? activeOption?.id : undefined,
+      children: value ? value : placeholder,


(nit)

Suggested change

children: value ? value : placeholder,

children: value ?? placeholder,

This actually creates a functional bug, where ?? accepts an empty string value and displays it instead of the placeholder. This especially makes a difference for multiselect, where there is always a string value calculated, so the placeholder would never show.

In that case, value || placeholder should work, right? Though what you have is fine too.

behowell · 2022-06-10T01:23:11Z

packages/react-components/react-combobox/src/ComboboxBase/useComboboxBaseSlots.ts

+  /* Add trigger props and event handlers */
+  let triggerSlot;
+
+  if ((state as ComboboxState).input) {


All of these as casts are not ideal. Does it work to do this instead?

Suggested change

if ((state as ComboboxState).input) {

if ('input' in state) {

That would (hopefully) let typescript figure out that the state object is ComboboxState, and let you avoid the as casts both in the if and else clauses.

More info: https://css-tricks.com/typescript-discriminated-unions/#aa-narrowing

That nifty TypeScript trick aside... it seems like maybe all of this logic doesn't belong in this function, since this is meant to be the shared logic. The Combobox stuff should ideally be in useCombobox, and the Dropdown stuff should ideally be in useDropdown.

I went back and forth on this a bit -- I could return a props object for the trigger and listbox and the event handlers directly instead of applying them to the slot, but it felt much less neat in practice. Do you have any strong opinions?

Another option is something simliar to what useARIAButton is doing, where the hook gets passed the shorthand props and options, and then calls resolveShorthand in the hook. The only issue there is how to get the proper typing for the ref, I think.

You could let the individual useCombobox/useDropdown hooks handle the default props for these slots (basically everything except ref I think?)

Then you could take a param triggerSlot: ComboboxState['input'] | DropdownState['button']. You might need to cast triggerRef to React.RefObject<unknown> or <never> to work here.

You could also skip taking it as a param and initialize it as:

const triggerSlot = ('input' in state) ? state.input : state.button;

behowell · 2022-06-10T01:31:29Z

packages/react-components/react-combobox/src/components/Combobox/Combobox.types.ts

-    value?: string;
+export type ComboboxProps = Omit<ComponentProps<Partial<ComboboxSlots>, 'input'>, 'children' | 'size'> &
+  ComboboxBaseProps & {
+    children: React.ReactNode;


Probably worth adding a doc comment here. Is the idea that there has to be exactly one child?

oh, nope! React.ReactNode can be one or multiple children. It's needed here b/c the primary slot is input, which doesn't allow children at all so it needs to be explicitly added. I think it might be better as Pick<ComponentProps<Partial<ComboboxSlots>, 'root'>, 'children'> instead

Got it! Maybe just add a comment that Combobox allows children even though its primary input slot doesn't. And I think the ReactNode type is fine... that other one is more confusing than helpful :)

behowell · 2022-06-10T01:34:49Z

packages/react-components/react-combobox/src/components/Combobox/useComboboxStyles.ts

+    // the SVG must have display: block for accurate positioning
+    // otherwise an extra inline space is inserted after the svg element
+    '& svg': {
+      display: 'block',
+    },


Is this a problem that could be common to all of our icon slots? Or is it something odd for just Combobox?

I'd expect it to be common to our icon slots that don't want an svg to be inline with text, and where extra space characters would matter.

unless something has changed with the icons -- I wrote this a while ago, and just copied it from ComboButton in this PR :D

behowell · 2022-06-10T01:36:59Z

packages/react-components/react-combobox/src/stories/ComboboxDefault.stories.tsx

-          <Option key={option} disabled={option === 'Ferret'}>
-            {option}
-          </Option>
+          <Option disabled={option === 'Ferret'}>{option}</Option>


Poor ferret 😔

behowell · 2022-06-10T18:11:28Z

packages/react-components/react-combobox/src/ComboboxBase/useComboboxBaseSlots.ts

+      'aria-expanded': open,
+      'aria-activedescendant': open ? activeOption?.id : undefined,
+      children: value ? value : placeholder,
+      role: 'combobox',


You're the last person I'd question about the role prop, but just to be sure it's not a copy/paste bug: is combobox the right role for Dropdown?

yup! We're calling it "Dropdown", but in ARIA terms it's a select-only combobox

behowell · 2022-06-10T18:32:28Z

packages/react-components/react-combobox/src/ComboboxBase/useComboboxBaseSlots.ts

+  const { onBlur: onTriggerBlur, onClick: onTriggerClick, onKeyDown: onTriggerKeyDown } = triggerSlot;
+  triggerSlot.onBlur = (event: React.FocusEvent<HTMLButtonElement> & React.FocusEvent<HTMLInputElement>) => {


You could use useMergedEventCallbacks for these, and skip defining/calling onTriggerBlur, etc.

Suggested change

const { onBlur: onTriggerBlur, onClick: onTriggerClick, onKeyDown: onTriggerKeyDown } = triggerSlot;

triggerSlot.onBlur = (event: React.FocusEvent<HTMLButtonElement> & React.FocusEvent<HTMLInputElement>) => {

triggerSlot.onBlur = useMergedEventCallbacks(triggerSlot.onBlur, event => {

smhigley · 2022-08-12T20:41:20Z

closed, resolved

smhigley added Component: Combobox Fluent UI react-components (v9) labels May 9, 2022

smhigley requested review from a team as code owners May 9, 2022 21:31

smhigley self-assigned this May 9, 2022

smhigley mentioned this pull request May 9, 2022

Combobox: child trigger/listbox pattern #22797

Closed

smhigley mentioned this pull request May 11, 2022

Combobox vNext: children vs. slots for listbox/trigger #22927

Closed

TristanWatanabe mentioned this pull request May 13, 2022

(chore) Move react-combobox to react-components subfolder #22830

Merged

smhigley marked this pull request as draft May 16, 2022 16:56

smhigley force-pushed the combobox-dropdown-slots branch from ece40e5 to bcc93cb Compare June 9, 2022 20:31

github-actions bot added this to the June Project Cycle Q2 2022 milestone Jun 9, 2022

smhigley added 9 commits June 9, 2022 16:30

add ComboboxInput component

f8908cc

ComboboxBase component

734211a

type wrangling

194c591

more type casting and wrangling

eace6fa

ref type, styles hook

a839098

remove idbase

9ccb405

rework slots to remove comboboxbase component

8ee198f

remove trigger components in favor of element slots

b4c1888

Fix icon slot error

b5eff56

smhigley force-pushed the combobox-dropdown-slots branch from bcc93cb to d05af96 Compare June 9, 2022 23:46

cleanup

3fe5f44

smhigley force-pushed the combobox-dropdown-slots branch from d05af96 to 3fe5f44 Compare June 9, 2022 23:49

behowell approved these changes Jun 10, 2022

View reviewed changes

smhigley mentioned this pull request Jun 10, 2022

react-combobox: remove ComboButton component #23492

Merged

behowell reviewed Jun 10, 2022

View reviewed changes

smhigley mentioned this pull request Jun 10, 2022

react-combobox: move Combobox functionality to shared hooks #23499

Merged

smhigley closed this Aug 12, 2022

	children: value ? value : placeholder,
	children: value ?? placeholder,

	if ((state as ComboboxState).input) {
	if ('input' in state) {

		const { onBlur: onTriggerBlur, onClick: onTriggerClick, onKeyDown: onTriggerKeyDown } = triggerSlot;
		triggerSlot.onBlur = (event: React.FocusEvent<HTMLButtonElement> & React.FocusEvent<HTMLInputElement>) => {

	const { onBlur: onTriggerBlur, onClick: onTriggerClick, onKeyDown: onTriggerKeyDown } = triggerSlot;
	triggerSlot.onBlur = (event: React.FocusEvent<HTMLButtonElement> & React.FocusEvent<HTMLInputElement>) => {
	triggerSlot.onBlur = useMergedEventCallbacks(triggerSlot.onBlur, event => {

Conversation

smhigley commented May 9, 2022

Uh oh!

fabricteam commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Bundle size report

Uh oh!

size-auditor bot commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Asset size changes

Uh oh!

fabricteam commented May 9, 2022

Perf Analysis (@fluentui/react-northstar)

Uh oh!

fabricteam commented May 9, 2022

Uh oh!

Hotell commented May 10, 2022

Uh oh!

codesandbox-ci bot commented Jun 9, 2022

Uh oh!

behowell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smhigley Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

behowell Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smhigley commented Aug 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fabricteam commented May 9, 2022 •

edited

Loading

size-auditor bot commented May 9, 2022 •

edited

Loading

Perf Analysis (`@fluentui/react-northstar`)

smhigley Jun 10, 2022 •

edited

Loading

behowell Jun 10, 2022 •

edited

Loading