RFC: Slot null rendering#18949
Conversation
|
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:
|
📊 Bundle size reportUnchanged fixtures
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 502b7758c2e102909d672452a5bfa024166e093f (build) |
Perf Analysis (
|
| 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 |
|
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 |
ecraig12345
left a comment
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Can you include a demo of calling the API with the special symbol?
There was a problem hiding this comment.
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.
Co-authored-by: ling1726 <lingfangao@hotmail.com>
Co-authored-by: ling1726 <lingfangao@hotmail.com>
Co-authored-by: Elizabeth Craig <ecraig12345@gmail.com>
fb195da to
7e451e6
Compare
| }; | ||
| ``` | ||
|
|
||
| ### Option 2: Symbol for null rendering |
There was a problem hiding this comment.
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:
- A slot that should only be rendered if the user provides content. For example, Checkbox's
labelslot. 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" />- 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'sinputslot:
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} />There was a problem hiding this comment.
One possible problem with that: what about the case of optional slot that have default properties?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
This is just a copy of the pro above, is that intended?
There was a problem hiding this comment.
Yes, it's intentional. Making shorthand logic more explicit can be considered a pro but also a con depending on the POV
There was a problem hiding this comment.
To avoid confusion, as I was also wondering, I would avoid repeating the same phrase and instead go with something like:
| 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 |
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>
layershifter
left a comment
There was a problem hiding this comment.
My vote is for Option 1 as it's less magical 😅
| }; | ||
| }; | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Optionally we can modify resolveShorthand's signature, to have second param as config:
resolveShorthand(props.slot2, {
optional: false,
defaultProps: { "aria-label": "foo" }
});There was a problem hiding this comment.
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
There was a problem hiding this comment.
Props in this case will be defined under a separate property (defaultProps), so they will not collide 😉
There was a problem hiding this comment.
Totally agree with this idea
There was a problem hiding this comment.
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>
theerebuss
left a comment
There was a problem hiding this comment.
Agree with @layershifter's point of going with Option 1.
|
@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 |
|
Sorry, totally forgot about that! we'll be going forward with Option 1, |
Pull request checklist
Description of changes
Proposes changes in slot handling mechanisms to solve problems on
null renderingscenariosPreview RFC