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 de1f3ee:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: e60678ee6153df90d5814982a6eddd381f7f6c28 (build) |
cceeab5 to
dcc1329
Compare
📊 Bundle size reportUnchanged fixtures
|
Perf Analysis (
|
| 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 |
31fae8a to
6650687
Compare
6650687 to
9eb4323
Compare
|
|
||
| ### 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_. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Would amending the original RFC and keeping one source of truth make sense? Unsure what the best way forward here is.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
This is definitely something to be discussed!
There was a problem hiding this comment.
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
Co-authored-by: ling1726 <lingfangao@hotmail.com>
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
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>
ecraig12345
left a comment
There was a problem hiding this comment.
Thanks for doing this! Some minor comments but generally LGTM.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Or, if root is a prop, what happens if I write:
<Component id="foo" root={{ id: "bar" }} />There was a problem hiding this comment.
Yes, it's ComponentProps responsibility to remove root from Slots and flatten it in Props itself.
There was a problem hiding this comment.
Or, if
rootis 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) |
There was a problem hiding this comment.
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" /> There was a problem hiding this comment.
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?
There was a problem hiding this comment.
If 'as' is added per-slot, then is
componentsa 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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
componentsis 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 👍
Description
This RFC proposes to treat
rootas a slot to simplify signature, improveTypingsand avoid conflicts betweenrootand other slots signatures.Main changes would be:
rootbecomes a regular slot on component stateasprop on every slotgetNativeElementPropson every slotPreview