Skip to content

RFC: Root as a slot#19068

Merged
bsunderhus merged 17 commits intomicrosoft:masterfrom
bsunderhus:rfc-root-as-slot
Aug 6, 2021
Merged

RFC: Root as a slot#19068
bsunderhus merged 17 commits intomicrosoft:masterfrom
bsunderhus:rfc-root-as-slot

Conversation

@bsunderhus
Copy link
Contributor

@bsunderhus bsunderhus commented Jul 22, 2021

Description

This RFC proposes to treat root as a slot to simplify signature, improve Typings and avoid conflicts between root and other slots signatures.

Main changes would be:

  1. root becomes a regular slot on component state
  2. Support references on every slot
  3. Supports Custom Elements on every slot
  4. Allows a restricted as prop on every slot
  5. stops using getNativeElementProps on every slot

Preview

@bsunderhus bsunderhus added the Type: RFC Request for Feedback label Jul 22, 2021
@bsunderhus bsunderhus requested review from a team July 22, 2021 11:22
@bsunderhus bsunderhus self-assigned this Jul 22, 2021
@bsunderhus bsunderhus added Status: Blocked Resolution blocked by another issue and removed Status: Blocked Resolution blocked by another issue labels Jul 22, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 22, 2021

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

Sandbox Source
Fluent UI React Starter Configuration

@size-auditor
Copy link

size-auditor bot commented Jul 22, 2021

Asset size changes

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

Baseline commit: e60678ee6153df90d5814982a6eddd381f7f6c28 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Jul 22, 2021

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-accordion
Accordion (including children components)
75.578 kB
22.287 kB
react-avatar
Avatar
56.558 kB
15.66 kB
react-badge
Badge
24.343 kB
7.165 kB
react-badge
CounterBadge
27.156 kB
7.851 kB
react-badge
PresenseBadge
237 B
177 B
react-button
Button
24.934 kB
8.001 kB
react-button
CompoundButton
30.226 kB
8.878 kB
react-button
MenuButton
26.521 kB
8.509 kB
react-button
ToggleButton
34.531 kB
8.637 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
176.958 kB
50.08 kB
react-components
react-components: FluentProvider & webLightTheme
36.237 kB
11.596 kB
react-divider
Divider
15.889 kB
5.747 kB
react-image
Image
10.642 kB
4.264 kB
react-label
Label
9.397 kB
3.839 kB
react-link
Link
14.715 kB
6.012 kB
react-make-styles
makeStaticStyles (runtime)
7.59 kB
3.321 kB
react-make-styles
makeStyles + mergeClasses (runtime)
22.135 kB
8.356 kB
react-make-styles
makeStyles + mergeClasses (build time)
2.557 kB
1.202 kB
react-menu
Menu (including children components)
114.61 kB
34.554 kB
react-menu
Menu (including selectable components)
116.71 kB
34.824 kB
react-popover
Popover
124.181 kB
36.121 kB
react-portal
Portal
7.78 kB
2.672 kB
react-positioning
usePopper
23.157 kB
7.942 kB
react-provider
FluentProvider
16.235 kB
5.972 kB
react-theme
Teams: all themes
32.941 kB
6.674 kB
react-theme
Teams: Light theme
20.247 kB
5.662 kB
react-tooltip
Tooltip
45.281 kB
15.45 kB
react-utilities
SSRProvider
213 B
170 B
🤖 This report was generated against e60678ee6153df90d5814982a6eddd381f7f6c28

@bsunderhus bsunderhus requested a review from ecraig12345 July 22, 2021 11:53
@fabricteam
Copy link
Collaborator

fabricteam commented Jul 22, 2021

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 839 863 5000
BaseButton mount 857 861 5000
Breadcrumb mount 2541 2459 1000
ButtonNext mount 407 398 5000
Checkbox mount 1400 1419 5000
CheckboxBase mount 1200 1204 5000
ChoiceGroup mount 4429 4417 5000
ComboBox mount 927 927 1000
CommandBar mount 9617 9553 1000
ContextualMenu mount 5863 5787 1000
DefaultButton mount 1070 1064 5000
DetailsRow mount 3506 3583 5000
DetailsRowFast mount 3452 3549 5000
DetailsRowNoStyles mount 3330 3297 5000
Dialog mount 2047 2024 1000
DocumentCardTitle mount 138 131 1000
Dropdown mount 3023 3026 5000
FluentProviderNext mount 7001 7140 5000
FocusTrapZone mount 1709 1690 5000
FocusZone mount 1659 1671 5000
IconButton mount 1626 1619 5000
Label mount 302 315 5000
Layer mount 1671 1670 5000
Link mount 438 443 5000
MakeStyles mount 1756 1708 50000
MenuButton mount 1386 1409 5000
MessageBar mount 1902 1924 5000
Nav mount 3123 3085 1000
OverflowSet mount 967 988 5000
Panel mount 1964 1991 1000
Persona mount 760 768 1000
Pivot mount 1308 1314 1000
PrimaryButton mount 1214 1195 5000
Rating mount 7185 7205 5000
SearchBox mount 1205 1240 5000
Shimmer mount 2357 2359 5000
Slider mount 1836 1832 5000
SpinButton mount 4703 4689 5000
Spinner mount 409 391 5000
SplitButton mount 2952 2998 5000
Stack mount 471 476 5000
StackWithIntrinsicChildren mount 1459 1465 5000
StackWithTextChildren mount 4233 4333 5000
SwatchColorPicker mount 9666 9667 5000
Tabs mount 1317 1317 1000
TagPicker mount 2428 2392 5000
TeachingBubble mount 11172 11325 5000
Text mount 400 381 5000
TextField mount 1273 1276 5000
ThemeProvider mount 1101 1103 5000
ThemeProvider virtual-rerender 563 553 5000
Toggle mount 741 761 5000
buttonNative mount 106 112 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AttachmentMinimalPerf.default 150 134 1.12:1
AccordionMinimalPerf.default 146 131 1.11:1
VideoMinimalPerf.default 601 548 1.1:1
TreeWith60ListItems.default 172 158 1.09:1
AlertMinimalPerf.default 255 243 1.05:1
AnimationMinimalPerf.default 401 381 1.05:1
AvatarMinimalPerf.default 183 175 1.05:1
ButtonSlotsPerf.default 526 506 1.04:1
ChatDuplicateMessagesPerf.default 277 267 1.04:1
FormMinimalPerf.default 378 365 1.04:1
HeaderSlotsPerf.default 722 694 1.04:1
ListNestedPerf.default 523 503 1.04:1
BoxMinimalPerf.default 327 319 1.03:1
LabelMinimalPerf.default 360 350 1.03:1
ReactionMinimalPerf.default 364 353 1.03:1
TooltipMinimalPerf.default 962 930 1.03:1
CarouselMinimalPerf.default 430 420 1.02:1
ChatMinimalPerf.default 608 599 1.02:1
DropdownManyItemsPerf.default 633 621 1.02:1
DropdownMinimalPerf.default 2974 2908 1.02:1
GridMinimalPerf.default 321 316 1.02:1
HeaderMinimalPerf.default 334 327 1.02:1
ItemLayoutMinimalPerf.default 1149 1121 1.02:1
ListCommonPerf.default 578 565 1.02:1
ListWith60ListItems.default 601 588 1.02:1
TextAreaMinimalPerf.default 470 463 1.02:1
ToolbarMinimalPerf.default 885 871 1.02:1
TreeMinimalPerf.default 752 735 1.02:1
EmbedMinimalPerf.default 3906 3877 1.01:1
ImageMinimalPerf.default 344 340 1.01:1
LoaderMinimalPerf.default 656 647 1.01:1
RefMinimalPerf.default 224 221 1.01:1
SkeletonMinimalPerf.default 328 324 1.01:1
SplitButtonMinimalPerf.default 3540 3518 1.01:1
ButtonOverridesMissPerf.default 1588 1592 1:1
CheckboxMinimalPerf.default 2574 2574 1:1
MenuButtonMinimalPerf.default 1528 1534 1:1
PopupMinimalPerf.default 555 557 1:1
ProviderMergeThemesPerf.default 1589 1586 1:1
ProviderMinimalPerf.default 943 945 1:1
RadioGroupMinimalPerf.default 408 409 1:1
StatusMinimalPerf.default 613 616 1:1
IconMinimalPerf.default 575 576 1:1
TableManyItemsPerf.default 1767 1769 1:1
TextMinimalPerf.default 323 323 1:1
CustomToolbarPrototype.default 3730 3731 1:1
AttachmentSlotsPerf.default 991 997 0.99:1
CardMinimalPerf.default 511 517 0.99:1
DialogMinimalPerf.default 709 717 0.99:1
InputMinimalPerf.default 1206 1216 0.99:1
ListMinimalPerf.default 470 473 0.99:1
TableMinimalPerf.default 372 376 0.99:1
ChatWithPopoverPerf.default 334 341 0.98:1
DividerMinimalPerf.default 338 345 0.98:1
FlexMinimalPerf.default 261 267 0.98:1
MenuMinimalPerf.default 781 799 0.98:1
SliderMinimalPerf.default 1472 1507 0.98:1
ButtonMinimalPerf.default 152 156 0.97:1
DatepickerMinimalPerf.default 4923 5058 0.97:1
RosterPerf.default 1068 1100 0.97:1
SegmentMinimalPerf.default 312 322 0.97:1
LayoutMinimalPerf.default 332 347 0.96:1
PortalMinimalPerf.default 158 165 0.96:1


### Make _slots_ support `as` property (but with restrictions)

Cancels [Do not support as prop for slots that render native DOM elements] as it already conflicts with [1st rule of ARIA opt-out mechanism]. This would add `as` property for `ObjectShorthandProps` signature for all _slots_.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we haven't had an RFC yet that undoes the result of a previous one... I wonder if we should create a new discarded folder for that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's even trickier than that, because it's not undoing the whole RFC, is undoing one of the alterations proposed by the RFC heheheh

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would amending the original RFC and keeping one source of truth make sense? Unsure what the best way forward here is.

Copy link
Contributor Author

@bsunderhus bsunderhus Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I kind of have this feeling that we're going to end up with a sea of RFC's and eventually some of them will end up conflicting somewhere.

I'd propose to go for the idea of having RFC's as a temporary thing and then having a more robust documentation that can be changed based on the proposals that come from the RFCs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely something to be discussed!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also thought about that. RFCs could be considered one-shot proposals only and we should port that into doc friendly format, with only how the user should do X

bsunderhus and others added 2 commits July 28, 2021 10:54
Co-authored-by: ling1726 <lingfangao@hotmail.com>
bsunderhus and others added 4 commits August 3, 2021 14:11
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
bsunderhus and others added 4 commits August 5, 2021 12:40
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
Co-authored-by: André Dias <andredias@microsoft.com>
Co-authored-by: André Dias <andredias@microsoft.com>
Copy link
Member

@ecraig12345 ecraig12345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! Some minor comments but generally LGTM.

bsunderhus and others added 4 commits August 6, 2021 09:59
Co-authored-by: Elizabeth Craig <ecraig12345@gmail.com>
Co-authored-by: Elizabeth Craig <ecraig12345@gmail.com>
Co-authored-by: Elizabeth Craig <ecraig12345@gmail.com>

Treating `root` as a slot means having to declare `root` _Typings_ together with other _slots_. This will impact in a components state signature, where `root` will have to be declared.

> ⚠️ That doesn't mean `root` will be available in component's properties interface
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify this about root not being in the properties interface? It looks like root is in the Slots type in the example below. Is there some magic happening in the ComponentProps<Slots> helper to remove root?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, if root is a prop, what happens if I write:

<Component id="foo" root={{ id: "bar" }} />

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's ComponentProps responsibility to remove root from Slots and flatten it in Props itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, if root is a prop, what happens if I write:

<Component id="foo" root={{ id: "bar" }} />

You can't do that, since root is not available as a property

}
```

### Make _slots_ support `as` property (but with restrictions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If 'as' is added per-slot, then is components a duplicate way of specifying the component type? E.g. are these two the same thing?

<Component label={{ as: 'span', children: 'foo' }} />
<Component components={{ label: 'span' }} label="foo" /> 

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been thinking about an alternative that would allow an optional tuple of [component, shorthandProps] for a slot, instead of the components prop. E.g., the equivalent to above would be:

<Component label={[ 'span', 'foo' ]} />

The benefit being that it still supports forwarding the as prop to a custom component:

<Component label={[ MyLabelWithAsProp, { as: 'span', children: 'foo' } ]} />

I wonder if that would be a possible alternative to supporting as natively in all slots?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would conflict with possible future RFC ideas like the one proposed by @smhigley #19266

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If 'as' is added per-slot, then is components a duplicate way of specifying the component type? E.g. are these two the same thing?

<Component label={{ as: 'span', children: 'foo' }} />
<Component components={{ label: 'span' }} label="foo" /> 

well, it would be... but components is also not available in the properties, it's state only

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also good to make it clear the main difference between as and component is what they support. as is limited to be a mechanism for semantic elements (change button to div), while component doesn't have that limitation, and that's the main reason why as is explicit and components isn't. The repercussion of making components somehow so accessible by the user would lean towards some undesired unmapped behaviors

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would conflict with possible future RFC ideas like the one proposed by @smhigley #19266

My idea was actually an attempt to solve a problem in that RFC--that you couldn't specify a different component per-slot--and would end up working nicely with her proposal. That said, Shift had some detailed comments on her RFC that might change the direction there (and obviate the problem I was trying to solve).

well, it would be... but components is also not available in the properties, it's state only

Ah, I misunderstood from #18642 -- I thought components was being exposed as a prop. Thanks for clarifying 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants