RFC: Primary slot updates #21007

ecraig12345 · 2021-12-13T21:36:29Z

Pull request checklist

Include a change request file using $ yarn change

Description of changes

This PR updates the primary slot RFC and implementation to address some issues myself, @behowell, and @micahgodbolt noticed while using the primary slot in our components.

The prop for the primary slot accepts ONLY className and style. Why?
- Eliminates the question of precedence
- Reduces some typing issues
The root prop accepts any native props EXCEPT className and style (and no shorthands--pretty sure the fact that it was previously accepting shorthands was an oversight)

fabricteam · 2021-12-13T21:43:49Z

📊 Bundle size report

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-theme` Teams: all themes	`29.344 kB` `6.55 kB`
`react-theme` Teams: Light theme	`18.4 kB` `5.273 kB`

_{🤖 This report was generated against 58c310891375b4ea87779d19241c48758910e852}

size-auditor · 2021-12-13T21:49:40Z

Asset size changes

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

Baseline commit: 58c310891375b4ea87779d19241c48758910e852 (build)

codesandbox-ci · 2021-12-13T21:52:03Z

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 551e385:

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

rfcs/convergence/native-element-props.md

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

behowell · 2021-12-14T17:15:07Z

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

+          [Key in keyof Primary]?: Pick<React.HTMLAttributes<{}>, 'className' | 'style'>;
+        } & {
+          // Add a `root` prop which accepts any native props, but NOT shorthand syntax
+          root?: Shorthands['root'];


@khmakoto is fixing a bunch of things mis-named Shorthand in another PR, but it might be good to include renaming this Shorthands template argument to Slots. Otherwise it's super confusing that the comment says it does NOT allow shorthand, and then declares it as "Shorthands" :)

Updated. Let me know if what I did is what you were intending.

behowell · 2021-12-14T17:20:02Z

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

-    Primary & 'root'
-  > &
-    PropsWithoutRef<Shorthands[Primary]>;
+      [Key in keyof Omit<Shorthands, Primary | 'root'>]?: ShorthandProps<NonNullable<Shorthands[Key]>>;


(nit) Exclude<keyof is a slightly more direct way of saying this instead of keyof Omit<. Internally Omit uses Exclude, so maybe just skip the step :)

Suggested change

[Key in keyof Omit<Shorthands, Primary | 'root'>]?: ShorthandProps<NonNullable<Shorthands[Key]>>;

[Key in Exclude<keyof Shorthands, Primary | 'root'>]?: ShorthandProps<NonNullable<Shorthands[Key]>>;

That gives me an error:

rfcs/convergence/native-element-props.md

behowell · 2021-12-14T17:26:12Z

rfcs/convergence/native-element-props.md

-  - `<Input className="foo" root={{ className: 'bar' }} />` => root element has `className="bar"`
-  - `<Input id="foo" input={{ id: 'bar' }} />` => input has `id="bar"`
+- Check that specifying `className` and `style` on the primary slot works
+- Check that `className` and `style` specified on the `root` slot take precedence over top-level props (with custom primary slot): `<Input className="foo" root={{ className: 'bar' }} />` => root element has `className="bar"`


I wonder if we should Omit className and style from the root slot prop, to avoid this precedence problem?

root?: Omit<Shorthands['root'], 'className' | 'style'>;

That's the main open question I had about these updates. Curious what others think.

rfcs/convergence/native-element-props.md

fabricteam · 2021-12-15T01:36:27Z

Perf Analysis (`@fluentui/react`)

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
ContextualMenu	mount	15429	7354	1000	Possible regression

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
Avatar	mount	801	766	5000
BaseButton	mount	725	823	5000
Breadcrumb	mount	2301	2311	1000
ButtonNext	mount	468	461	5000
Checkbox	mount	1343	2684	5000
CheckboxBase	mount	1068	1078	5000
ChoiceGroup	mount	4085	4152	5000
ComboBox	mount	837	821	1000
CommandBar	mount	8764	8719	1000
ContextualMenu	mount	15429	7354	1000	Possible regression
DefaultButton	mount	971	996	5000
DetailsRow	mount	3166	3268	5000
DetailsRowFast	mount	3247	3247	5000
DetailsRowNoStyles	mount	3010	6181	5000
Dialog	mount	2130	2106	1000
DocumentCardTitle	mount	153	162	1000
Dropdown	mount	2696	2775	5000
FluentProviderNext	mount	1636	1562	5000
FluentProviderWithTheme	mount	135	136	10
FluentProviderWithTheme	virtual-rerender	103	93	10
FluentProviderWithTheme	virtual-rerender-with-unmount	140	166	10
FocusTrapZone	mount	1561	1577	5000
FocusZone	mount	1564	1584	5000
IconButton	mount	1515	1426	5000
Label	mount	336	326	5000
Layer	mount	2520	2924	5000
Link	mount	429	382	5000
MakeStyles	mount	1615	1612	50000
MenuButton	mount	1263	1307	5000
MessageBar	mount	1739	1754	5000
Nav	mount	2868	4578	1000
OverflowSet	mount	971	998	5000
Panel	mount	2151	2208	1000
Persona	mount	737	759	1000
Pivot	mount	1264	1261	1000
PrimaryButton	mount	1145	1150	5000
Rating	mount	6664	6685	5000
SearchBox	mount	1175	1155	5000
Shimmer	mount	2185	2196	5000
Slider	mount	1724	1709	5000
SpinButton	mount	4284	4319	5000
Spinner	mount	415	395	5000
SplitButton	mount	2682	2745	5000
Stack	mount	445	454	5000
StackWithIntrinsicChildren	mount	1471	1468	5000
StackWithTextChildren	mount	3886	3946	5000
SwatchColorPicker	mount	8955	8930	5000
TagPicker	mount	2260	2291	5000
TeachingBubble	mount	11114	11014	5000
Text	mount	343	390	5000
TextField	mount	1215	1214	5000
ThemeProvider	mount	1052	1046	5000
ThemeProvider	virtual-rerender	548	544	5000
ThemeProvider	virtual-rerender-with-unmount	1608	1621	5000
Toggle	mount	724	736	5000
buttonNative	mount	132	148	5000

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

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
MenuMinimalPerf.default	1887	696	2.71:1
InputMinimalPerf.default	2627	1090	2.41:1
ButtonMinimalPerf.default	148	125	1.18:1
ListMinimalPerf.default	439	386	1.14:1
AccordionMinimalPerf.default	132	119	1.11:1
PortalMinimalPerf.default	168	153	1.1:1
SegmentMinimalPerf.default	309	287	1.08:1
VideoMinimalPerf.default	577	535	1.08:1
FlexMinimalPerf.default	261	245	1.07:1
MenuButtonMinimalPerf.default	1390	1313	1.06:1
RefMinimalPerf.default	218	206	1.06:1
AlertMinimalPerf.default	237	226	1.05:1
BoxMinimalPerf.default	291	276	1.05:1
IconMinimalPerf.default	537	513	1.05:1
TextMinimalPerf.default	306	291	1.05:1
TextAreaMinimalPerf.default	438	419	1.05:1
ButtonSlotsPerf.default	482	463	1.04:1
CarouselMinimalPerf.default	412	395	1.04:1
ChatDuplicateMessagesPerf.default	261	250	1.04:1
ChatWithPopoverPerf.default	342	329	1.04:1
TableMinimalPerf.default	352	340	1.04:1
TreeMinimalPerf.default	684	659	1.04:1
ChatMinimalPerf.default	645	625	1.03:1
DialogMinimalPerf.default	635	615	1.03:1
ImageMinimalPerf.default	326	316	1.03:1
SkeletonMinimalPerf.default	305	297	1.03:1
SliderMinimalPerf.default	1439	1402	1.03:1
TreeWith60ListItems.default	146	142	1.03:1
ButtonOverridesMissPerf.default	1401	1367	1.02:1
DropdownMinimalPerf.default	2489	2449	1.02:1
HeaderSlotsPerf.default	660	648	1.02:1
ReactionMinimalPerf.default	323	318	1.02:1
AttachmentMinimalPerf.default	138	137	1.01:1
DatepickerMinimalPerf.default	4747	4721	1.01:1
LayoutMinimalPerf.default	309	307	1.01:1
ProviderMergeThemesPerf.default	1477	1467	1.01:1
TableManyItemsPerf.default	1595	1587	1.01:1
TooltipMinimalPerf.default	890	878	1.01:1
AnimationMinimalPerf.default	371	370	1:1
CardMinimalPerf.default	474	475	1:1
FormMinimalPerf.default	352	351	1:1
LoaderMinimalPerf.default	585	584	1:1
ToolbarMinimalPerf.default	742	740	1:1
DropdownManyItemsPerf.default	570	573	0.99:1
ItemLayoutMinimalPerf.default	969	977	0.99:1
LabelMinimalPerf.default	332	335	0.99:1
ListCommonPerf.default	536	540	0.99:1
RosterPerf.default	995	1004	0.99:1
ProviderMinimalPerf.default	882	895	0.99:1
SplitButtonMinimalPerf.default	3594	3627	0.99:1
StatusMinimalPerf.default	571	577	0.99:1
CheckboxMinimalPerf.default	2220	2275	0.98:1
EmbedMinimalPerf.default	3363	3435	0.98:1
DividerMinimalPerf.default	306	315	0.97:1
RadioGroupMinimalPerf.default	351	363	0.97:1
PopupMinimalPerf.default	491	513	0.96:1
AvatarMinimalPerf.default	157	166	0.95:1
ListNestedPerf.default	432	457	0.95:1
ListWith60ListItems.default	526	558	0.94:1
GridMinimalPerf.default	265	289	0.92:1
HeaderMinimalPerf.default	277	306	0.91:1
AttachmentSlotsPerf.default	802	913	0.88:1
CustomToolbarPrototype.default	3437	4009	0.86:1

bsunderhus

LGTM,

Regarding the open question, I'm in favor of removing className and styles of root on the scenarios where root is not the primary slot to ensure we don't need to worry about precedence or merging anything.

ecraig12345 · 2021-12-15T17:09:38Z

We now have two votes in favor of removing className and style from the root slot prop, so I'll probably go ahead with that unless anyone objects.

ecraig12345 · 2021-12-16T00:46:48Z

Updated to ban className and style on the root slot prop.

However, this change is causing typing issues in useCheckbox and useInput that I'm having trouble figuring out (the restricted types in props seem to be creating some mismatches with the current way resolveShorthand is typed). Would appreciate any help from @behowell or others.

msft-fluent-ui-bot · 2022-09-03T00:00:04Z

Because this pull request has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

The pull request will still be available for reference. If it's still relevant to merge at some point, you can reopen or make a new version based on the latest code.

ecraig12345 added Fluent UI react-components (v9) Type: RFC Request for Feedback labels Dec 13, 2021

ecraig12345 requested review from bsunderhus and behowell December 13, 2021 21:36

ecraig12345 requested a review from a team as a code owner December 13, 2021 21:36

msft-fluent-ui-bot assigned layershifter Dec 13, 2021

khmakoto approved these changes Dec 14, 2021

View reviewed changes

rfcs/convergence/native-element-props.md Show resolved Hide resolved

behowell approved these changes Dec 14, 2021

View reviewed changes

ecraig12345 force-pushed the primary-slot-updates branch from 2936044 to ecea9d7 Compare December 15, 2021 01:07

ecraig12345 changed the title ~~Primary slot updates~~ RFC: Primary slot updates Dec 15, 2021

ecraig12345 mentioned this pull request Dec 15, 2021

Input: implement primary slot #20863

Merged

2 tasks

bsunderhus approved these changes Dec 15, 2021

View reviewed changes

ecraig12345 added 3 commits December 15, 2021 15:54

Update primary slot props

e1d4227

PR feedback

5a2cdd5

ban classname and style on root

551e385

ecraig12345 force-pushed the primary-slot-updates branch from ecea9d7 to 551e385 Compare December 16, 2021 00:44

raghavthind2005 approved these changes Apr 5, 2022

View reviewed changes

ecraig12345 assigned khmakoto and unassigned layershifter Apr 5, 2022

msft-fluent-ui-bot added the Resolution: Soft Close Soft closing inactive issues over a certain period label Sep 3, 2022

msft-fluent-ui-bot closed this Sep 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Primary slot updates #21007

RFC: Primary slot updates #21007

ecraig12345 commented Dec 13, 2021 •

edited

fabricteam commented Dec 13, 2021 •

edited

size-auditor bot commented Dec 13, 2021 •

edited

codesandbox-ci bot commented Dec 13, 2021 •

edited

behowell Dec 14, 2021

ecraig12345 Dec 15, 2021

behowell Dec 14, 2021

ecraig12345 Dec 15, 2021

behowell Dec 14, 2021

ecraig12345 Dec 15, 2021

fabricteam commented Dec 15, 2021

bsunderhus left a comment

ecraig12345 commented Dec 15, 2021 •

edited

ecraig12345 commented Dec 16, 2021

msft-fluent-ui-bot commented Sep 3, 2022

	[Key in keyof Omit<Shorthands, Primary \| 'root'>]?: ShorthandProps<NonNullable<Shorthands[Key]>>;
	[Key in Exclude<keyof Shorthands, Primary \| 'root'>]?: ShorthandProps<NonNullable<Shorthands[Key]>>;

RFC: Primary slot updates #21007

RFC: Primary slot updates #21007

Conversation

ecraig12345 commented Dec 13, 2021 • edited

Pull request checklist

Description of changes

fabricteam commented Dec 13, 2021 • edited

📊 Bundle size report

size-auditor bot commented Dec 13, 2021 • edited

Asset size changes

codesandbox-ci bot commented Dec 13, 2021 • edited

behowell Dec 14, 2021

Choose a reason for hiding this comment

ecraig12345 Dec 15, 2021

Choose a reason for hiding this comment

behowell Dec 14, 2021

Choose a reason for hiding this comment

ecraig12345 Dec 15, 2021

Choose a reason for hiding this comment

behowell Dec 14, 2021

Choose a reason for hiding this comment

ecraig12345 Dec 15, 2021

Choose a reason for hiding this comment

fabricteam commented Dec 15, 2021

Perf Analysis (@fluentui/react)

Perf Analysis (@fluentui/react-northstar)

bsunderhus left a comment

Choose a reason for hiding this comment

ecraig12345 commented Dec 15, 2021 • edited

ecraig12345 commented Dec 16, 2021

msft-fluent-ui-bot commented Sep 3, 2022

ecraig12345 commented Dec 13, 2021 •

edited

fabricteam commented Dec 13, 2021 •

edited

size-auditor bot commented Dec 13, 2021 •

edited

codesandbox-ci bot commented Dec 13, 2021 •

edited

Perf Analysis (`@fluentui/react`)

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

ecraig12345 commented Dec 15, 2021 •

edited