Skip to content

RFC: Slot null rendering#18949

Merged
bsunderhus merged 17 commits intomicrosoft:masterfrom
bsunderhus:slot-null-rendering
Aug 5, 2021
Merged

RFC: Slot null rendering#18949
bsunderhus merged 17 commits intomicrosoft:masterfrom
bsunderhus:slot-null-rendering

Conversation

@bsunderhus
Copy link
Contributor

@bsunderhus bsunderhus commented Jul 15, 2021

Pull request checklist

Description of changes

Proposes changes in slot handling mechanisms to solve problems on null rendering scenarios

Preview RFC

@bsunderhus bsunderhus added the Type: RFC Request for Feedback label Jul 15, 2021
@bsunderhus bsunderhus requested review from a team and ecraig12345 July 15, 2021 11:38
@bsunderhus bsunderhus self-assigned this Jul 15, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 15, 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 814084b:

Sandbox Source
Fluent UI React Starter Configuration

@fabricteam
Copy link
Collaborator

fabricteam commented Jul 15, 2021

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-accordion
Accordion (including children components)
78.506 kB
23.21 kB
react-avatar
Avatar
54.242 kB
14.662 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
179.799 kB
50.935 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 502b7758c2e102909d672452a5bfa024166e093f

@size-auditor
Copy link

size-auditor bot commented Jul 15, 2021

Asset size changes

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

Baseline commit: 502b7758c2e102909d672452a5bfa024166e093f (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Jul 15, 2021

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 871 872 5000
BaseButton mount 970 1001 5000
Breadcrumb mount 2707 2770 1000
ButtonNext mount 442 453 5000
Checkbox mount 1659 1666 5000
CheckboxBase mount 1446 1406 5000
ChoiceGroup mount 5204 5320 5000
ComboBox mount 1010 1041 1000
CommandBar mount 10768 10505 1000
ContextualMenu mount 6557 6601 1000
DefaultButton mount 1260 1245 5000
DetailsRow mount 3985 3983 5000
DetailsRowFast mount 3811 3942 5000
DetailsRowNoStyles mount 3741 3667 5000
Dialog mount 2262 2213 1000
DocumentCardTitle mount 163 177 1000
Dropdown mount 3420 3432 5000
FluentProviderNext mount 7373 7075 5000
FocusTrapZone mount 1847 1862 5000
FocusZone mount 1805 1842 5000
IconButton mount 1923 1866 5000
Label mount 362 334 5000
Layer mount 1907 1899 5000
Link mount 503 489 5000
MakeStyles mount 1833 1798 50000
MenuButton mount 1622 1628 5000
MessageBar mount 2113 2141 5000
Nav mount 3445 3451 1000
OverflowSet mount 1111 1144 5000
Panel mount 2186 2109 1000
Persona mount 884 886 1000
Pivot mount 1522 1497 1000
PrimaryButton mount 1453 1370 5000
Rating mount 8419 8468 5000
SearchBox mount 1500 1449 5000
Shimmer mount 2845 2856 5000
Slider mount 2075 2109 5000
SpinButton mount 5342 5334 5000
Spinner mount 433 435 5000
SplitButton mount 3481 3518 5000
Stack mount 547 552 5000
StackWithIntrinsicChildren mount 1689 1715 5000
StackWithTextChildren mount 5080 5071 5000
SwatchColorPicker mount 10955 10832 5000
Tabs mount 1510 1534 1000
TagPicker mount 2709 2746 5000
TeachingBubble mount 12385 12478 5000
Text mount 454 450 5000
TextField mount 1517 1507 5000
ThemeProvider mount 1332 1217 5000
ThemeProvider virtual-rerender 614 636 5000
Toggle mount 885 880 5000
buttonNative mount 109 117 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
BoxMinimalPerf.default 383 351 1.09:1
ListNestedPerf.default 647 592 1.09:1
DividerMinimalPerf.default 406 375 1.08:1
CardMinimalPerf.default 612 572 1.07:1
VideoMinimalPerf.default 696 648 1.07:1
AnimationMinimalPerf.default 453 427 1.06:1
ProviderMinimalPerf.default 1120 1055 1.06:1
SkeletonMinimalPerf.default 385 363 1.06:1
TextAreaMinimalPerf.default 581 549 1.06:1
TreeWith60ListItems.default 198 186 1.06:1
ChatMinimalPerf.default 702 671 1.05:1
ReactionMinimalPerf.default 411 392 1.05:1
RefMinimalPerf.default 237 225 1.05:1
ButtonMinimalPerf.default 181 176 1.03:1
ItemLayoutMinimalPerf.default 1294 1262 1.03:1
PopupMinimalPerf.default 605 587 1.03:1
SegmentMinimalPerf.default 392 379 1.03:1
StatusMinimalPerf.default 749 728 1.03:1
TableMinimalPerf.default 439 426 1.03:1
TextMinimalPerf.default 364 354 1.03:1
ToolbarMinimalPerf.default 1016 982 1.03:1
AttachmentMinimalPerf.default 171 167 1.02:1
ButtonOverridesMissPerf.default 1824 1781 1.02:1
DropdownMinimalPerf.default 3187 3134 1.02:1
ListWith60ListItems.default 686 672 1.02:1
MenuButtonMinimalPerf.default 1771 1737 1.02:1
RosterPerf.default 1334 1303 1.02:1
CarouselMinimalPerf.default 490 486 1.01:1
ChatDuplicateMessagesPerf.default 310 308 1.01:1
CheckboxMinimalPerf.default 2904 2879 1.01:1
GridMinimalPerf.default 348 345 1.01:1
HeaderSlotsPerf.default 823 815 1.01:1
InputMinimalPerf.default 1314 1300 1.01:1
MenuMinimalPerf.default 928 919 1.01:1
PortalMinimalPerf.default 181 179 1.01:1
TreeMinimalPerf.default 840 832 1.01:1
AttachmentSlotsPerf.default 1151 1151 1:1
DialogMinimalPerf.default 798 795 1:1
DropdownManyItemsPerf.default 749 751 1:1
EmbedMinimalPerf.default 4256 4251 1:1
FlexMinimalPerf.default 293 294 1:1
LabelMinimalPerf.default 405 406 1:1
ListCommonPerf.default 705 706 1:1
LoaderMinimalPerf.default 739 737 1:1
ProviderMergeThemesPerf.default 1704 1698 1:1
TableManyItemsPerf.default 2105 2099 1:1
FormMinimalPerf.default 454 459 0.99:1
HeaderMinimalPerf.default 398 402 0.99:1
RadioGroupMinimalPerf.default 487 491 0.99:1
SliderMinimalPerf.default 1623 1642 0.99:1
CustomToolbarPrototype.default 3994 4033 0.99:1
AlertMinimalPerf.default 291 297 0.98:1
ButtonSlotsPerf.default 586 596 0.98:1
LayoutMinimalPerf.default 389 398 0.98:1
IconMinimalPerf.default 643 656 0.98:1
TooltipMinimalPerf.default 1037 1054 0.98:1
ChatWithPopoverPerf.default 389 402 0.97:1
DatepickerMinimalPerf.default 5619 5767 0.97:1
ListMinimalPerf.default 559 577 0.97:1
AvatarMinimalPerf.default 198 206 0.96:1
ImageMinimalPerf.default 386 406 0.95:1
AccordionMinimalPerf.default 151 170 0.89:1

@ling1726
Copy link
Contributor

I would vote for Option 1 and check for presence of shorthand, this actually makes sense to me especially if a slot is intended to be optional

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, but as someone who's new to working with the composition helpers, I feel like there's some context missing for me to be able to understand what's being proposed (so it's hard to evaluate the options). Some specific questions below.


### Option 2: Symbol for null rendering

This solves this problem by verifying if the shorthand has a special symbol or not.
Copy link
Member

Choose a reason for hiding this comment

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

Can you include a demo of calling the API with the special symbol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The special symbol would only be used inside the methods declared to create shorthands and slots, meaning the methods resolveShorthand and getSlots from react-utilities, it should not leak for the component development itself.

bsunderhus and others added 6 commits July 20, 2021 12:45
@bsunderhus bsunderhus force-pushed the slot-null-rendering branch from fb195da to 7e451e6 Compare July 20, 2021 10:45
@bsunderhus bsunderhus requested review from a team and ling1726 July 22, 2021 08:43
@bsunderhus bsunderhus requested review from a team and behowell July 23, 2021 06:50
@bsunderhus bsunderhus requested a review from ecraig12345 July 23, 2021 08:47
};
```

### Option 2: Symbol for null rendering
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of option 2, but why not use null as the indicator of a slot that shouldn't be rendered?

The two cases are:

  1. A slot that should only be rendered if the user provides content. For example, Checkbox's label slot. In this case, the defaultProps argument to resolveShorthand could be null:
label: resolveShorthand(props.label, null),

If props.label is defined, that'd override the null default:

<Checkbox label="Example" />
  1. A slot that should always be rendered by default, even if the user doesn't provide content. In that case, the defaultProps argument would be non-null (an object with defaults, or an empty object {} if there are no additional defaults). For example, Checkbox's input slot:
input: resolveShorthand(props.input, { type: 'checkbox' }),

The user could override and prevent rendering the slot by passing null. This doesn't make sense in the case of Checkbox, but just as a demo:

<Checkbox input={null} />

Copy link
Contributor Author

@bsunderhus bsunderhus Jul 26, 2021

Choose a reason for hiding this comment

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

One possible problem with that: what about the case of optional slot that have default properties?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's case 2 in my example above. It provides default props { type: 'checkbox' }, but can be explicitly set to null by the user: input={null}. To implement, resolveShorthand would just check the first argument for null and return null in that case.

There is a case 3, which is a slot that always must be rendered and can never be prevented from rendering (e.g. root). In that case, we'd simply not include null in the possible type for that slot.

alwaysRenderedSlot: React.HTMLAttributes<HTMLElement>;
optionallyRenderedSlot: React.HTMLAttributes<HTMLElement> | null;


### Cons

1. This takes the responsibility of deciding if a slot might be null rendered out of inner implementation of `getSlot` into the developers hand, by providing a optional way of declaring it
Copy link
Member

Choose a reason for hiding this comment

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

This is just a copy of the pro above, is that intended?

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 intentional. Making shorthand logic more explicit can be considered a pro but also a con depending on the POV

Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid confusion, as I was also wondering, I would avoid repeating the same phrase and instead go with something like:

Suggested change
1. This takes the responsibility of deciding if a slot might be null rendered out of inner implementation of `getSlot` into the developers hand, by providing a optional way of declaring it
1. Same as the pros, depending on the POV

bsunderhus and others added 9 commits July 30, 2021 18:46
Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

My vote is for Option 1 as it's less magical 😅

};
};
```

Copy link
Member

Choose a reason for hiding this comment

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

Optionally we can modify resolveShorthand's signature, to have second param as config:

resolveShorthand(props.slot2, {
  optional: false,
  defaultProps: { "aria-label": "foo" }
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm, there's always the possibility that optional is actually a property from the slot, and that would generate a conflict hehehe, but yeah, we could assume that slots can't have optional as prop

Copy link
Member

Choose a reason for hiding this comment

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

Props in this case will be defined under a separate property (defaultProps), so they will not collide 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree with this idea

Copy link
Contributor

@Hotell Hotell Aug 3, 2021

Choose a reason for hiding this comment

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

what @layershifter said , adding any kind of positional arguments makes API less readable/hard to maintain/brittle to breaking changes

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
Copy link
Contributor

@theerebuss theerebuss left a comment

Choose a reason for hiding this comment

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

Agree with @layershifter's point of going with Option 1.

@bsunderhus bsunderhus merged commit 5c7f5bd into microsoft:master Aug 5, 2021
@bsunderhus bsunderhus deleted the slot-null-rendering branch August 5, 2021 10:51
@Hotell
Copy link
Contributor

Hotell commented Aug 5, 2021

@bsunderhus can you please send new PR that will mark which solution we are going forward ? I'm not seeing that being updated in this PR. thx

@bsunderhus
Copy link
Contributor Author

bsunderhus commented Aug 5, 2021

Sorry, totally forgot about that! we'll be going forward with Option 1,
I'll update in a new PR

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.

10 participants