Add Primary slot infrastructure and support to Checkbox #20617

behowell · 2021-11-15T20:58:41Z

Pull request checklist

Addresses an existing issue: Fixes Project: Implement primary slot in Checkbox #20547
Include a change request file using $ yarn change

Description of changes

Add support for the primary slot described in RFC RFC: Customizing application of native element props and ref (RFC: Customizing application of native element props and ref #18983)
- Add getRootSlotNativeProps and getPrimarySlotNativeProps helper functions to make sure the right props go to the right slots
- Update conformance tests to support primarySlot
- Update utility types
Change Checkbox's primary slot to input, and other misc cleanup for Checkbox

…x to use the primary slot

…heckbox-primary-slot

size-auditor · 2021-11-15T21:13:23Z

Asset size changes

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

Baseline commit: 8b8e00f0d0bf82ee665f63bd33a2f314fc1e0e28 (build)

codesandbox-ci · 2021-11-15T21:17:51Z

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 ea0bd7c:

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

fabricteam · 2021-11-15T21:20:27Z

📊 Bundle size report

_{🤖 This report was generated against 8b8e00f0d0bf82ee665f63bd33a2f314fc1e0e28}

fabricteam · 2021-11-15T21:34:50Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	853	894	5000
BaseButton	mount	874	911	5000
Breadcrumb	mount	2388	2374	1000
ButtonNext	mount	543	546	5000
Checkbox	mount	1398	1474	5000
CheckboxBase	mount	1191	1181	5000
ChoiceGroup	mount	4150	4175	5000
ComboBox	mount	964	942	1000
CommandBar	mount	8874	8950	1000
ContextualMenu	mount	7474	7403	1000
DefaultButton	mount	1061	1079	5000
DetailsRow	mount	3416	3352	5000
DetailsRowFast	mount	3357	3333	5000
DetailsRowNoStyles	mount	3154	3211	5000
Dialog	mount	2281	2299	1000
DocumentCardTitle	mount	243	240	1000
Dropdown	mount	2867	2872	5000
FluentProviderNext	mount	3670	3652	5000
FluentProviderWithTheme	mount	289	297	10
FluentProviderWithTheme	virtual-rerender	194	196	10
FluentProviderWithTheme	virtual-rerender-with-unmount	341	370	10
FocusTrapZone	mount	1659	1631	5000
FocusZone	mount	1741	1659	5000
IconButton	mount	1628	1611	5000
Label	mount	391	395	5000
Layer	mount	2653	2651	5000
Link	mount	517	523	5000
MakeStyles	mount	1653	1653	50000
MenuButton	mount	1362	1337	5000
MessageBar	mount	1800	1815	5000
Nav	mount	2926	2903	1000
OverflowSet	mount	1050	1063	5000
Panel	mount	2204	2224	1000
Persona	mount	850	809	1000
Pivot	mount	1350	1331	1000
PrimaryButton	mount	1210	1192	5000
Rating	mount	6634	6761	5000
SearchBox	mount	1244	1236	5000
Shimmer	mount	2322	2229	5000
Slider	mount	1767	1775	5000
SpinButton	mount	4364	4402	5000
Spinner	mount	481	465	5000
SplitButton	mount	2798	2776	5000
Stack	mount	542	553	5000
StackWithIntrinsicChildren	mount	1546	1524	5000
StackWithTextChildren	mount	4086	4077	5000
SwatchColorPicker	mount	9048	9067	5000
TagPicker	mount	2336	2335	5000
TeachingBubble	mount	11387	11323	5000
Text	mount	466	467	5000
TextField	mount	1292	1307	5000
ThemeProvider	mount	1135	1119	5000
ThemeProvider	virtual-rerender	614	627	5000
ThemeProvider	virtual-rerender-with-unmount	1679	1680	5000
Toggle	mount	805	829	5000
buttonNative	mount	209	224	5000

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

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
DropdownManyItemsPerf.default	627	579	1.08:1
ChatDuplicateMessagesPerf.default	276	257	1.07:1
AvatarMinimalPerf.default	181	171	1.06:1
FormMinimalPerf.default	363	345	1.05:1
CardMinimalPerf.default	491	473	1.04:1
CarouselMinimalPerf.default	414	399	1.04:1
FlexMinimalPerf.default	261	251	1.04:1
HeaderMinimalPerf.default	315	304	1.04:1
AnimationMinimalPerf.default	362	351	1.03:1
LayoutMinimalPerf.default	323	313	1.03:1
ListNestedPerf.default	486	470	1.03:1
LoaderMinimalPerf.default	597	579	1.03:1
RadioGroupMinimalPerf.default	394	382	1.03:1
AlertMinimalPerf.default	236	231	1.02:1
ButtonMinimalPerf.default	151	148	1.02:1
ChatWithPopoverPerf.default	331	325	1.02:1
DialogMinimalPerf.default	663	647	1.02:1
GridMinimalPerf.default	292	286	1.02:1
ListMinimalPerf.default	445	437	1.02:1
PopupMinimalPerf.default	516	507	1.02:1
RefMinimalPerf.default	213	208	1.02:1
ButtonSlotsPerf.default	478	471	1.01:1
ChatMinimalPerf.default	564	561	1.01:1
CheckboxMinimalPerf.default	2271	2243	1.01:1
InputMinimalPerf.default	1124	1111	1.01:1
ItemLayoutMinimalPerf.default	1026	1014	1.01:1
LabelMinimalPerf.default	326	322	1.01:1
MenuButtonMinimalPerf.default	1387	1374	1.01:1
PortalMinimalPerf.default	159	158	1.01:1
ReactionMinimalPerf.default	331	328	1.01:1
SplitButtonMinimalPerf.default	3654	3630	1.01:1
TableMinimalPerf.default	347	343	1.01:1
TextAreaMinimalPerf.default	422	417	1.01:1
TreeWith60ListItems.default	149	147	1.01:1
AttachmentMinimalPerf.default	134	134	1:1
AttachmentSlotsPerf.default	926	923	1:1
BoxMinimalPerf.default	298	299	1:1
ButtonOverridesMissPerf.default	1442	1442	1:1
DatepickerMinimalPerf.default	4756	4762	1:1
DividerMinimalPerf.default	312	311	1:1
HeaderSlotsPerf.default	636	637	1:1
ListWith60ListItems.default	562	563	1:1
MenuMinimalPerf.default	730	728	1:1
RosterPerf.default	1020	1015	1:1
ProviderMergeThemesPerf.default	1480	1477	1:1
ProviderMinimalPerf.default	968	972	1:1
SkeletonMinimalPerf.default	302	303	1:1
SliderMinimalPerf.default	1421	1419	1:1
StatusMinimalPerf.default	572	572	1:1
TableManyItemsPerf.default	1602	1599	1:1
TextMinimalPerf.default	294	294	1:1
ToolbarMinimalPerf.default	806	805	1:1
TreeMinimalPerf.default	674	675	1:1
AccordionMinimalPerf.default	139	140	0.99:1
DropdownMinimalPerf.default	2561	2582	0.99:1
EmbedMinimalPerf.default	3499	3527	0.99:1
IconMinimalPerf.default	530	533	0.99:1
CustomToolbarPrototype.default	3502	3525	0.99:1
TooltipMinimalPerf.default	879	885	0.99:1
VideoMinimalPerf.default	545	550	0.99:1
ImageMinimalPerf.default	319	326	0.98:1
ListCommonPerf.default	536	553	0.97:1
SegmentMinimalPerf.default	296	304	0.97:1

bsunderhus

LGTM my only problem is with the early generalization being introduced into react-utilities, and the fact that the names of those methods are too generic for such a specific use case, this might cause a lot of confusion

packages/react-checkbox/src/components/Checkbox/useCheckbox.tsx

packages/react-utilities/src/utils/getNativeElementProps.ts

…heckbox-primary-slot

ecraig12345 · 2021-11-19T22:52:49Z

packages/react-checkbox/src/components/Checkbox/Checkbox.types.ts

@@ -68,13 +68,8 @@ export type CheckboxSlots = {
 /**
 * Checkbox Props
 */
-export type CheckboxProps = Omit<ComponentProps<CheckboxSlots>, 'defaultChecked'> &
+export type CheckboxProps = Omit<ComponentProps<CheckboxSlots, 'input'>, 'size' | 'checked' | 'defaultChecked'> &


I think for consistency with other fields and to eliminate the native prop overlap issue, we should consider renaming size to fieldSize. That's what I have in Input and plan to do in other input components for consistency.

Could be a good idea, but that's out of scope of this PR. Could you make a PR that makes the change for all input components?

Made an issue for now, #20733

ecraig12345

Thanks for doing this! Bunch of minor comments but it looks good overall.

packages/react-checkbox/src/components/Checkbox/Checkbox.types.ts

ecraig12345 · 2021-11-19T23:36:56Z

packages/react-checkbox/src/components/Checkbox/renderCheckbox.tsx


 export const renderCheckbox = (state: CheckboxState) => {
-  const { slots, slotProps } = getSlots<CheckboxSlots>(state, checkboxShorthandProps);
+  const { slots, slotProps } = getSlots<CheckboxSlots>(state, ['root', 'indicator', 'input']);


Mainly just curious, what was the reasoning for removing the checkboxShorthandProps const?

It's only used in one place; it seems unnecessary. Unless it's worthwhile to export it publicly?

I guess I was mainly thinking it could make things a little more self-documenting? Definitely doesn't need to be exported.

It seems straightforward that the argument to the getSlots function is the name of the slots, at least to me.

packages/react-checkbox/src/components/Checkbox/useCheckbox.tsx

packages/react-checkbox/src/components/Checkbox/Checkbox.types.ts

packages/react-conformance/src/defaultErrorMessages.tsx

packages/react-utilities/src/utils/getNativeElementProps.ts

…luentui into checkbox-cleanup

…heckbox-primary-slot

…luentui into checkbox-cleanup

behowell · 2021-11-23T00:21:15Z

@bsunderhus @layershifter @ecraig12345 I've replied to your comments and updated the PR. Could you take another look and sign off if it looks ok? Thanks.

packages/react-checkbox/src/components/Checkbox/useCheckbox.tsx

packages/react-conformance/src/types.ts

packages/react-utilities/src/compose/types.ts

…heckbox-primary-slot

…luentui into checkbox-primary-slot

) Add support for the primary slot described in RFC [RFC: Customizing application of native element props and ref](https://github.com/microsoft/fluentui/blob/master/rfcs/convergence/native-element-props.md) (microsoft#18983) * Add `getPartitionedNativeProps` helper function to make sure the right props go to the right slots * Update conformance tests to support primarySlot * Update utility types * Change Checkbox's primary slot to `input`, and other misc cleanup for Checkbox

behowell added 4 commits November 15, 2021 12:36

Add infrastructure and tests for the primary slot, and update checkbo…

e56f4af

…x to use the primary slot

Merge branch 'master' of https://github.com/microsoft/fluentui into c…

9ad8694

…heckbox-primary-slot

Misc cleanup

cb81d2b

Change files

acbfeaf

behowell requested review from ecraig12345, khmakoto and a team November 15, 2021 20:58

behowell self-assigned this Nov 15, 2021

behowell requested a review from a team as a code owner November 15, 2021 20:58

msft-fluent-ui-bot added the Component: Checkbox label Nov 15, 2021

bsunderhus requested changes Nov 16, 2021

View changes

msft-fluent-ui-bot added the Needs: Author Feedback label Nov 16, 2021

layershifter reviewed Nov 16, 2021

View changes

packages/react-utilities/src/utils/getNativeElementProps.ts Outdated Show resolved Hide resolved

Merge branch 'master' of https://github.com/microsoft/fluentui into c…

56ed144

…heckbox-primary-slot

msft-fluent-ui-bot removed the Needs: Author Feedback label Nov 18, 2021

behowell added 4 commits November 18, 2021 17:58

Update from PR feedback

c343ddf

Add conformance test for primary slot

97c8a7f

Fix inputRef if the ref is assigned on the input slot

f6c53fe

Merge branch 'master' of https://github.com/microsoft/fluentui into c…

f873b05

…heckbox-primary-slot

ecraig12345 reviewed Nov 19, 2021

View changes

behowell added 2 commits November 19, 2021 15:33

Clean up checkbox unit tests

2763230

Clean up default checkmark icon

90870fb

ecraig12345 reviewed Nov 19, 2021

View changes

behowell added 3 commits November 19, 2021 16:24

Add hasLabel to state instead of looking at children

a2f6fba

Refactor styles to use css vars for indicator

3a156de

Fix paddnig between indicator and label

67d5c07

behowell added 5 commits November 19, 2021 16:27

Merge branch 'checkbox-primary-slot' of https://github.com/behowell/f…

660a1eb

…luentui into checkbox-cleanup

Merge branch 'master' of https://github.com/microsoft/fluentui into c…

454ff30

…heckbox-primary-slot

PR feedback

be4bf4b

Merge branch 'checkbox-primary-slot' of https://github.com/behowell/f…

4bb5ce1

…luentui into checkbox-cleanup

Rename indicator css variables

6ffae3a

khmakoto reviewed Nov 23, 2021

View changes

packages/react-checkbox/src/components/Checkbox/useCheckbox.tsx Outdated Show resolved Hide resolved

packages/react-conformance/src/types.ts Show resolved Hide resolved

packages/react-utilities/src/compose/types.ts Outdated Show resolved Hide resolved

layershifter approved these changes Nov 23, 2021

View changes

bsunderhus approved these changes Nov 23, 2021

View changes

ecraig12345 added this to In progress in CXE Redmond - @microsoft/cxe-red via automation Nov 23, 2021

ecraig12345 added this to In progress in !!! OLD DO NOT USE!!! v9.0.0 via automation Nov 23, 2021

behowell added 3 commits November 24, 2021 12:02

Comment cleanup from PR feedback

5d94b3e

Merge branch 'master' of https://github.com/microsoft/fluentui into c…

bf8c27a

…heckbox-primary-slot

Merge branch 'checkbox-primary-slot' of https://github.com/behowell/f…

3b5eb06

…luentui into checkbox-primary-slot

khmakoto approved these changes Nov 24, 2021

View changes

Update comment in ComponentProps to clarify tr

ea0bd7c

behowell merged commit 99adff5 into microsoft:master Nov 24, 2021
23 checks passed

CXE Redmond - @microsoft/cxe-red automation moved this from In progress to Done Nov 24, 2021

!!! OLD DO NOT USE!!! v9.0.0 automation moved this from In progress to Done Nov 24, 2021

This was referenced Nov 29, 2021

Implement primary slot/native props RFC #19868

Closed

Project: Get Checkbox ready for release #20589

Closed

behowell deleted the checkbox-primary-slot branch July 25, 2022 17:37

Add Primary slot infrastructure and support to Checkbox #20617

Add Primary slot infrastructure and support to Checkbox #20617

behowell commented Nov 15, 2021

size-auditor bot commented Nov 15, 2021 •

edited

codesandbox-ci bot commented Nov 15, 2021 •

edited

fabricteam commented Nov 15, 2021 •

edited

fabricteam commented Nov 15, 2021 •

edited

bsunderhus left a comment

ecraig12345 Nov 19, 2021

behowell Nov 22, 2021

ecraig12345 Nov 23, 2021

ecraig12345 left a comment

ecraig12345 Nov 19, 2021

behowell Nov 22, 2021

ecraig12345 Nov 23, 2021

behowell Nov 24, 2021

behowell commented Nov 23, 2021

Add Primary slot infrastructure and support to Checkbox #20617

Add Primary slot infrastructure and support to Checkbox #20617

Conversation

behowell commented Nov 15, 2021

Pull request checklist

Description of changes

size-auditor bot commented Nov 15, 2021 • edited

Asset size changes

codesandbox-ci bot commented Nov 15, 2021 • edited

fabricteam commented Nov 15, 2021 • edited

📊 Bundle size report

fabricteam commented Nov 15, 2021 • edited

Perf Analysis (@fluentui/react-northstar)

bsunderhus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecraig12345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behowell commented Nov 23, 2021

size-auditor bot commented Nov 15, 2021 •

edited

codesandbox-ci bot commented Nov 15, 2021 •

edited

fabricteam commented Nov 15, 2021 •

edited

fabricteam commented Nov 15, 2021 •

edited

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