refactor(Badge): Remove mergeProps and migrate to simple slots#19763
refactor(Badge): Remove mergeProps and migrate to simple slots#19763ling1726 merged 14 commits intomicrosoft:masterfrom
Conversation
📊 Bundle size reportUnchanged fixtures
|
|
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 7169f69:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 1efd41e2ec1086eba6d970c05202845b75b5cc22 (build) |
| if (typeof badge === 'string') { | ||
| badge = { as: PresenceBadge, status: badge }; | ||
| // typing should be fixed after mergeProps migration with the `components` property in state | ||
| badge = { as: (PresenceBadge as unknown) as 'div', status: badge }; |
There was a problem hiding this comment.
This should fixed in #19449 with the separation of as and components
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 880 | 904 | 5000 | |
| BaseButton | mount | 861 | 875 | 5000 | |
| Breadcrumb | mount | 2600 | 2591 | 1000 | |
| ButtonNext | mount | 440 | 447 | 5000 | |
| Checkbox | mount | 1477 | 1476 | 5000 | |
| CheckboxBase | mount | 1247 | 1261 | 5000 | |
| ChoiceGroup | mount | 4659 | 4610 | 5000 | |
| ComboBox | mount | 1029 | 957 | 1000 | |
| CommandBar | mount | 10162 | 10304 | 1000 | |
| ContextualMenu | mount | 6520 | 6489 | 1000 | |
| DefaultButton | mount | 1122 | 1089 | 5000 | |
| DetailsRow | mount | 3626 | 3652 | 5000 | |
| DetailsRowFast | mount | 3672 | 3664 | 5000 | |
| DetailsRowNoStyles | mount | 3488 | 3496 | 5000 | |
| Dialog | mount | 2404 | 2429 | 1000 | |
| DocumentCardTitle | mount | 138 | 141 | 1000 | |
| Dropdown | mount | 3118 | 3171 | 5000 | |
| FluentProviderNext | mount | 7517 | 7366 | 5000 | |
| FluentProviderWithTheme | mount | 349 | 355 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 103 | 100 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 509 | 505 | 10 | |
| FocusTrapZone | mount | 1820 | 1739 | 5000 | |
| FocusZone | mount | 1815 | 1796 | 5000 | |
| IconButton | mount | 1699 | 1735 | 5000 | |
| Label | mount | 331 | 323 | 5000 | |
| Layer | mount | 3011 | 2925 | 5000 | |
| Link | mount | 449 | 461 | 5000 | |
| MakeStyles | mount | 1805 | 1812 | 50000 | |
| MenuButton | mount | 1454 | 1435 | 5000 | |
| MessageBar | mount | 2002 | 1972 | 5000 | |
| Nav | mount | 3256 | 3210 | 1000 | |
| OverflowSet | mount | 1113 | 1117 | 5000 | |
| Panel | mount | 1350 | 2363 | 1000 | |
| Persona | mount | 818 | 832 | 1000 | |
| Pivot | mount | 1397 | 1406 | 1000 | |
| PrimaryButton | mount | 1266 | 1250 | 5000 | |
| Rating | mount | 7537 | 7425 | 5000 | |
| SearchBox | mount | 1290 | 1304 | 5000 | |
| Shimmer | mount | 2494 | 2487 | 5000 | |
| Slider | mount | 1953 | 1957 | 5000 | |
| SpinButton | mount | 4946 | 4919 | 5000 | |
| Spinner | mount | 415 | 411 | 5000 | |
| SplitButton | mount | 3131 | 3343 | 5000 | |
| Stack | mount | 479 | 484 | 5000 | |
| StackWithIntrinsicChildren | mount | 1507 | 1532 | 5000 | |
| StackWithTextChildren | mount | 4469 | 4469 | 5000 | |
| SwatchColorPicker | mount | 10194 | 10048 | 5000 | |
| Tabs | mount | 1374 | 1373 | 1000 | |
| TagPicker | mount | 2540 | 2565 | 5000 | |
| TeachingBubble | mount | 13088 | 13182 | 5000 | |
| Text | mount | 409 | 418 | 5000 | |
| TextField | mount | 1327 | 1332 | 5000 | |
| ThemeProvider | mount | 1147 | 1181 | 5000 | |
| ThemeProvider | virtual-rerender | 602 | 611 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1860 | 1861 | 5000 | |
| Toggle | mount | 781 | 787 | 5000 | |
| buttonNative | mount | 116 | 118 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| FormMinimalPerf.default | 404 | 377 | 1.07:1 |
| GridMinimalPerf.default | 345 | 326 | 1.06:1 |
| SegmentMinimalPerf.default | 356 | 337 | 1.06:1 |
| HeaderMinimalPerf.default | 361 | 343 | 1.05:1 |
| SkeletonMinimalPerf.default | 363 | 347 | 1.05:1 |
| StatusMinimalPerf.default | 674 | 642 | 1.05:1 |
| AttachmentMinimalPerf.default | 152 | 146 | 1.04:1 |
| ImageMinimalPerf.default | 373 | 358 | 1.04:1 |
| LayoutMinimalPerf.default | 374 | 361 | 1.04:1 |
| TextMinimalPerf.default | 347 | 335 | 1.04:1 |
| AnimationMinimalPerf.default | 409 | 398 | 1.03:1 |
| ChatDuplicateMessagesPerf.default | 287 | 279 | 1.03:1 |
| ListNestedPerf.default | 554 | 536 | 1.03:1 |
| PortalMinimalPerf.default | 176 | 171 | 1.03:1 |
| RadioGroupMinimalPerf.default | 449 | 436 | 1.03:1 |
| RefMinimalPerf.default | 249 | 241 | 1.03:1 |
| SliderMinimalPerf.default | 1593 | 1544 | 1.03:1 |
| ToolbarMinimalPerf.default | 931 | 905 | 1.03:1 |
| DropdownManyItemsPerf.default | 676 | 664 | 1.02:1 |
| ItemLayoutMinimalPerf.default | 1196 | 1169 | 1.02:1 |
| ListMinimalPerf.default | 520 | 511 | 1.02:1 |
| MenuMinimalPerf.default | 839 | 824 | 1.02:1 |
| PopupMinimalPerf.default | 607 | 593 | 1.02:1 |
| TableManyItemsPerf.default | 1906 | 1877 | 1.02:1 |
| AccordionMinimalPerf.default | 156 | 154 | 1.01:1 |
| AvatarMinimalPerf.default | 189 | 187 | 1.01:1 |
| ButtonOverridesMissPerf.default | 1690 | 1677 | 1.01:1 |
| ButtonSlotsPerf.default | 531 | 528 | 1.01:1 |
| DatepickerMinimalPerf.default | 5496 | 5415 | 1.01:1 |
| HeaderSlotsPerf.default | 751 | 746 | 1.01:1 |
| LabelMinimalPerf.default | 385 | 381 | 1.01:1 |
| ProviderMergeThemesPerf.default | 1687 | 1672 | 1.01:1 |
| VideoMinimalPerf.default | 623 | 619 | 1.01:1 |
| AttachmentSlotsPerf.default | 1044 | 1048 | 1:1 |
| BoxMinimalPerf.default | 343 | 343 | 1:1 |
| ButtonMinimalPerf.default | 169 | 169 | 1:1 |
| ChatMinimalPerf.default | 640 | 642 | 1:1 |
| DialogMinimalPerf.default | 748 | 745 | 1:1 |
| DropdownMinimalPerf.default | 3086 | 3078 | 1:1 |
| EmbedMinimalPerf.default | 4074 | 4091 | 1:1 |
| InputMinimalPerf.default | 1251 | 1249 | 1:1 |
| ListWith60ListItems.default | 627 | 627 | 1:1 |
| LoaderMinimalPerf.default | 685 | 682 | 1:1 |
| SplitButtonMinimalPerf.default | 4125 | 4128 | 1:1 |
| TextAreaMinimalPerf.default | 485 | 485 | 1:1 |
| TooltipMinimalPerf.default | 995 | 993 | 1:1 |
| CarouselMinimalPerf.default | 441 | 445 | 0.99:1 |
| CheckboxMinimalPerf.default | 2732 | 2756 | 0.99:1 |
| MenuButtonMinimalPerf.default | 1621 | 1633 | 0.99:1 |
| RosterPerf.default | 1139 | 1151 | 0.99:1 |
| ProviderMinimalPerf.default | 998 | 1011 | 0.99:1 |
| ReactionMinimalPerf.default | 369 | 372 | 0.99:1 |
| CustomToolbarPrototype.default | 3828 | 3856 | 0.99:1 |
| TreeMinimalPerf.default | 774 | 781 | 0.99:1 |
| TreeWith60ListItems.default | 175 | 177 | 0.99:1 |
| CardMinimalPerf.default | 533 | 546 | 0.98:1 |
| FlexMinimalPerf.default | 277 | 282 | 0.98:1 |
| ListCommonPerf.default | 601 | 615 | 0.98:1 |
| TableMinimalPerf.default | 394 | 404 | 0.98:1 |
| AlertMinimalPerf.default | 272 | 280 | 0.97:1 |
| ChatWithPopoverPerf.default | 347 | 358 | 0.97:1 |
| IconMinimalPerf.default | 594 | 611 | 0.97:1 |
| DividerMinimalPerf.default | 350 | 370 | 0.95:1 |
| * @defaultvalue medium | ||
| * @default medium |
There was a problem hiding this comment.
Out of curiosity, did we ever make a final decision on @default vs. @defaultvalue vs. @defaultValue? I think there was some problem with using @default with TSDoc.
The last I heard last month, it was still open for discussion in the v-build team: #19271 (comment)
There was a problem hiding this comment.
No idea tbh, I don't think it really matters until we decide to stick with one officially.
packages/react-badge/src/components/CounterBadge/CounterBadge.types.ts
Outdated
Show resolved
Hide resolved
| * @default filled | ||
| */ | ||
| count: number; | ||
| appearance: Extract<BadgeProps['appearance'], 'filled' | 'ghost'>; |
There was a problem hiding this comment.
Ditto
| appearance: Extract<BadgeProps['appearance'], 'filled' | 'ghost'>; | |
| appearance: 'filled' | 'ghost'; |
| /** | ||
| * {@docCategory CounterBadge} | ||
| */ | ||
| export interface CounterBadgeProps extends Omit<BadgeProps, 'appearance' | 'shape'>, Partial<CounterBadgeCommons> {} |
There was a problem hiding this comment.
Rather than extending BadgeProps directly, what do you think about having each badge define its own Slots and Commons types that equal or extend BadgeSlots/BadgeCommons?
export interface CounterBadgeSlots extends BadgeSlots {}
export interface CounterBadgeCommons extends Omit<BadgeCommons, 'appearance' | 'shape'> {
// ...
}
export interface CounterBadgeProps extends ComponentProps<Partial<CounterBadgeSlots>>, Partial<CounterBadgeCommons> {}
export interface CounterBadgeState extends ComponentState<CounterBadgeSlots>, CounterBadgeCommons {}There was a problem hiding this comment.
I just don't see a need for extra types at this point, is there a reason it would be better to have separate types ? is it for API consistency ?
There was a problem hiding this comment.
The CounterBadgeSlots type is purely for consistency. It could be left out if desired.
The other half of my suggestion is to have CounterBadgeCommons extend BadgeCommons, rather than having the Props/State types extend the base badge. That makes it so you only need to use Omit for 'appearance' | 'shape' once, and it seems more logical to me, at least.
packages/react-badge/src/components/CounterBadge/useCounterBadge.ts
Outdated
Show resolved
Hide resolved
…soft#19763) * refactor(Badge): Remove `mergeProps` and migrate to simple slots * Change files * update API * fix import * chore(Avatar): typing workaround for presence badge * Change files * Update packages/react-avatar/src/components/Avatar/useAvatar.tsx * pr suggestions * remove unnecessary cast * remove Extract types * update md
Pull request checklist
$ yarn changeDescription of changes
Removes mergeProps and migrates to simple slots
Focus areas to test
Existing tests should pass