Skip to content

refactor(Badge): Remove mergeProps and migrate to simple slots#19763

Merged
ling1726 merged 14 commits intomicrosoft:masterfrom
ling1726:chore/badge-slot-migration
Sep 21, 2021
Merged

refactor(Badge): Remove mergeProps and migrate to simple slots#19763
ling1726 merged 14 commits intomicrosoft:masterfrom
ling1726:chore/badge-slot-migration

Conversation

@ling1726
Copy link
Contributor

Pull request checklist

Description of changes

Removes mergeProps and migrates to simple slots

Focus areas to test

Existing tests should pass

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 13, 2021

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-avatar
Avatar
59.048 kB
16.636 kB
57.803 kB
16.148 kB
1.245 kB
488 B
react-badge
Badge
23.535 kB
6.893 kB
23.939 kB
7.029 kB
-404 B
-136 B
react-badge
CounterBadge
26.702 kB
7.635 kB
26.929 kB
7.743 kB
-227 B
-108 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-badge
PresenseBadge
237 B
177 B
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
164.296 kB
46.863 kB
react-components
react-components: FluentProvider & webLightTheme
35.754 kB
11.393 kB
🤖 This report was generated against 1efd41e2ec1086eba6d970c05202845b75b5cc22

@ling1726 ling1726 marked this pull request as ready for review September 13, 2021 09:21
@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 13, 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 7169f69:

Sandbox Source
Fluent UI React Starter Configuration

@size-auditor
Copy link

size-auditor bot commented Sep 13, 2021

Asset size changes

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

Baseline commit: 1efd41e2ec1086eba6d970c05202845b75b5cc22 (build)

@ling1726 ling1726 requested a review from a team as a code owner September 13, 2021 12:00
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 };
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 should fixed in #19449 with the separation of as and components

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 13, 2021

Perf Analysis (@fluentui/react)

No significant results to display.

All results

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

Comment on lines +38 to +40
* @defaultvalue medium
* @default medium
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea tbh, I don't think it really matters until we decide to stick with one officially.

* @default filled
*/
count: number;
appearance: Extract<BadgeProps['appearance'], 'filled' | 'ghost'>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Suggested change
appearance: Extract<BadgeProps['appearance'], 'filled' | 'ghost'>;
appearance: 'filled' | 'ghost';

/**
* {@docCategory CounterBadge}
*/
export interface CounterBadgeProps extends Omit<BadgeProps, 'appearance' | 'shape'>, Partial<CounterBadgeCommons> {}
Copy link
Contributor

Choose a reason for hiding this comment

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

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 {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

@behowell behowell Sep 16, 2021

Choose a reason for hiding this comment

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

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.

@ling1726 ling1726 enabled auto-merge (squash) September 21, 2021 15:25
@ling1726 ling1726 merged commit 0ab9827 into microsoft:master Sep 21, 2021
mlp73 pushed a commit to mlp73/fluentui that referenced this pull request Jan 17, 2022
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

react-badge: use simplified prop merging

5 participants