-
Notifications
You must be signed in to change notification settings - Fork 2.9k
react-avatar: AvatarGroup SPEC review #22523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
react-avatar: AvatarGroup SPEC review #22523
Conversation
|
|
||
| There are three layout variants in AvatarGroup: | ||
|
|
||
| - Grid layout (Default): Avatars are spaced evenly and there can be a maximum of five of them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there any visuals you can link here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could take a screenshot of the design spec?
📊 Bundle size report🤖 This report was generated against f65f187799f50793c9263cb299d0426a28cc4e9a |
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: f65f187799f50793c9263cb299d0426a28cc4e9a (build) |
|
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 7109f28:
|
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Button | mount | 570 | 543 | 5000 | Possible regression |
| FluentProviderWithTheme | mount | 279 | 281 | 10 | Possible regression |
| FluentProviderWithTheme | virtual-rerender | 212 | 251 | 10 | Possible regression |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 287 | 325 | 10 | Possible regression |
All results
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 906 | 905 | 5000 | |
| Button | mount | 570 | 543 | 5000 | Possible regression |
| FluentProvider | mount | 1790 | 1892 | 5000 | |
| FluentProviderWithTheme | mount | 279 | 281 | 10 | Possible regression |
| FluentProviderWithTheme | virtual-rerender | 212 | 251 | 10 | Possible regression |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 287 | 325 | 10 | Possible regression |
| MakeStyles | mount | 1580 | 1543 | 50000 |
packages/react-avatar/src/components/AvatarGroup/AvatarGroup.types.ts
Outdated
Show resolved
Hide resolved
packages/react-avatar/src/components/AvatarGroup/AvatarGroup.types.ts
Outdated
Show resolved
Hide resolved
packages/react-avatar/src/components/AvatarGroup/useAvatarGroupStyles.ts
Outdated
Show resolved
Hide resolved
Perf Analysis (
|
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| AttachmentMinimalPerf.default | 164 | 144 | 1.14:1 |
| TextMinimalPerf.default | 357 | 326 | 1.1:1 |
| CarouselMinimalPerf.default | 495 | 454 | 1.09:1 |
| LayoutMinimalPerf.default | 379 | 347 | 1.09:1 |
| ListWith60ListItems.default | 696 | 636 | 1.09:1 |
| TreeWith60ListItems.default | 180 | 165 | 1.09:1 |
| FlexMinimalPerf.default | 289 | 272 | 1.06:1 |
| ImageMinimalPerf.default | 391 | 368 | 1.06:1 |
| ToolbarMinimalPerf.default | 964 | 909 | 1.06:1 |
| AttachmentSlotsPerf.default | 1135 | 1084 | 1.05:1 |
| HeaderSlotsPerf.default | 765 | 732 | 1.05:1 |
| RefMinimalPerf.default | 239 | 228 | 1.05:1 |
| ButtonSlotsPerf.default | 564 | 544 | 1.04:1 |
| GridMinimalPerf.default | 351 | 337 | 1.04:1 |
| HeaderMinimalPerf.default | 363 | 349 | 1.04:1 |
| ListMinimalPerf.default | 523 | 505 | 1.04:1 |
| ButtonOverridesMissPerf.default | 1515 | 1473 | 1.03:1 |
| DialogMinimalPerf.default | 773 | 752 | 1.03:1 |
| ProviderMergeThemesPerf.default | 1309 | 1276 | 1.03:1 |
| ProviderMinimalPerf.default | 405 | 393 | 1.03:1 |
| RadioGroupMinimalPerf.default | 454 | 441 | 1.03:1 |
| ReactionMinimalPerf.default | 381 | 369 | 1.03:1 |
| ItemLayoutMinimalPerf.default | 1208 | 1180 | 1.02:1 |
| RosterPerf.default | 1115 | 1097 | 1.02:1 |
| TableManyItemsPerf.default | 1951 | 1904 | 1.02:1 |
| TableMinimalPerf.default | 401 | 392 | 1.02:1 |
| TextAreaMinimalPerf.default | 492 | 483 | 1.02:1 |
| AlertMinimalPerf.default | 265 | 263 | 1.01:1 |
| AnimationMinimalPerf.default | 544 | 537 | 1.01:1 |
| BoxMinimalPerf.default | 333 | 331 | 1.01:1 |
| ChatMinimalPerf.default | 743 | 738 | 1.01:1 |
| CheckboxMinimalPerf.default | 2709 | 2695 | 1.01:1 |
| DropdownManyItemsPerf.default | 668 | 664 | 1.01:1 |
| FormMinimalPerf.default | 416 | 410 | 1.01:1 |
| ListCommonPerf.default | 630 | 622 | 1.01:1 |
| PopupMinimalPerf.default | 642 | 635 | 1.01:1 |
| PortalMinimalPerf.default | 171 | 169 | 1.01:1 |
| SliderMinimalPerf.default | 1703 | 1687 | 1.01:1 |
| VideoMinimalPerf.default | 627 | 620 | 1.01:1 |
| ChatDuplicateMessagesPerf.default | 288 | 289 | 1:1 |
| EmbedMinimalPerf.default | 4157 | 4148 | 1:1 |
| InputMinimalPerf.default | 1297 | 1301 | 1:1 |
| MenuButtonMinimalPerf.default | 1700 | 1697 | 1:1 |
| StatusMinimalPerf.default | 674 | 673 | 1:1 |
| CustomToolbarPrototype.default | 2743 | 2747 | 1:1 |
| TooltipMinimalPerf.default | 1057 | 1061 | 1:1 |
| TreeMinimalPerf.default | 800 | 797 | 1:1 |
| AvatarMinimalPerf.default | 187 | 188 | 0.99:1 |
| DropdownMinimalPerf.default | 3066 | 3105 | 0.99:1 |
| IconMinimalPerf.default | 587 | 594 | 0.99:1 |
| ButtonMinimalPerf.default | 160 | 164 | 0.98:1 |
| CardMinimalPerf.default | 546 | 558 | 0.98:1 |
| DatepickerMinimalPerf.default | 5740 | 5841 | 0.98:1 |
| LabelMinimalPerf.default | 352 | 358 | 0.98:1 |
| ListNestedPerf.default | 533 | 544 | 0.98:1 |
| MenuMinimalPerf.default | 838 | 854 | 0.98:1 |
| SkeletonMinimalPerf.default | 332 | 338 | 0.98:1 |
| SplitButtonMinimalPerf.default | 4504 | 4597 | 0.98:1 |
| ChatWithPopoverPerf.default | 386 | 400 | 0.97:1 |
| DividerMinimalPerf.default | 337 | 348 | 0.97:1 |
| LoaderMinimalPerf.default | 684 | 706 | 0.97:1 |
| SegmentMinimalPerf.default | 334 | 358 | 0.93:1 |
| AccordionMinimalPerf.default | 135 | 148 | 0.91:1 |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| TeachingBubble | mount | 49191 | 89682 | 5000 | Possible regression |
All results
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| BaseButton | mount | 801 | 779 | 5000 | |
| Breadcrumb | mount | 2362 | 2355 | 1000 | |
| Checkbox | mount | 1285 | 1272 | 5000 | |
| CheckboxBase | mount | 1075 | 1106 | 5000 | |
| ChoiceGroup | mount | 4043 | 4029 | 5000 | |
| ComboBox | mount | 840 | 861 | 1000 | |
| CommandBar | mount | 9112 | 9196 | 1000 | |
| ContextualMenu | mount | 10047 | 10544 | 1000 | |
| DefaultButton | mount | 974 | 980 | 5000 | |
| DetailsRow | mount | 3318 | 3305 | 5000 | |
| DetailsRowFast | mount | 3362 | 3295 | 5000 | |
| DetailsRowNoStyles | mount | 3142 | 3181 | 5000 | |
| Dialog | mount | 1946 | 1918 | 1000 | |
| DocumentCardTitle | mount | 159 | 146 | 1000 | |
| Dropdown | mount | 2833 | 2860 | 5000 | |
| FocusTrapZone | mount | 1593 | 1550 | 5000 | |
| FocusZone | mount | 1583 | 1594 | 5000 | |
| IconButton | mount | 1461 | 1516 | 5000 | |
| Label | mount | 308 | 301 | 5000 | |
| Layer | mount | 2568 | 2513 | 5000 | |
| Link | mount | 407 | 404 | 5000 | |
| MenuButton | mount | 1262 | 1303 | 5000 | |
| MessageBar | mount | 1827 | 1741 | 5000 | |
| Nav | mount | 2848 | 2896 | 1000 | |
| OverflowSet | mount | 933 | 939 | 5000 | |
| Panel | mount | 1842 | 1865 | 1000 | |
| Persona | mount | 870 | 881 | 1000 | |
| Pivot | mount | 1214 | 1179 | 1000 | |
| PrimaryButton | mount | 1119 | 1136 | 5000 | |
| Rating | mount | 6707 | 6761 | 5000 | |
| SearchBox | mount | 1127 | 1138 | 5000 | |
| Shimmer | mount | 2164 | 2175 | 5000 | |
| Slider | mount | 1690 | 1690 | 5000 | |
| SpinButton | mount | 4373 | 4367 | 5000 | |
| Spinner | mount | 387 | 380 | 5000 | |
| SplitButton | mount | 2730 | 2729 | 5000 | |
| Stack | mount | 445 | 451 | 5000 | |
| StackWithIntrinsicChildren | mount | 1968 | 1961 | 5000 | |
| StackWithTextChildren | mount | 4412 | 4486 | 5000 | |
| SwatchColorPicker | mount | 10013 | 10045 | 5000 | |
| TagPicker | mount | 2293 | 2335 | 5000 | |
| TeachingBubble | mount | 49191 | 89682 | 5000 | Possible regression |
| Text | mount | 350 | 369 | 5000 | |
| TextField | mount | 1205 | 1224 | 5000 | |
| ThemeProvider | mount | 994 | 1021 | 5000 | |
| ThemeProvider | virtual-rerender | 568 | 565 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1644 | 1595 | 5000 | |
| Toggle | mount | 699 | 688 | 5000 | |
| buttonNative | mount | 107 | 114 | 5000 |
packages/react-avatar/src/components/AvatarGroup/AvatarGroup.types.ts
Outdated
Show resolved
Hide resolved
packages/react-avatar/src/components/AvatarGroup/AvatarGroup.types.ts
Outdated
Show resolved
Hide resolved
packages/react-avatar/src/components/AvatarGroup/AvatarGroup.types.ts
Outdated
Show resolved
Hide resolved
packages/react-avatar/src/components/AvatarGroup/AvatarGroup.types.ts
Outdated
Show resolved
Hide resolved
packages/react-avatar/src/components/AvatarGroup/AvatarGroup.types.ts
Outdated
Show resolved
Hide resolved
packages/react-avatar/src/components/AvatarGroup/AvatarGroup.types.ts
Outdated
Show resolved
Hide resolved
packages/react-avatar/src/components/AvatarGroup/AvatarGroup.types.ts
Outdated
Show resolved
Hide resolved
| tooltipContent: TooltipProps['content']; | ||
| }; | ||
|
|
||
| export type AvatarGroupStrings = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smhigley Would appreciate your thoughts there as you wrote an RFC on this topic.
khmakoto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this PR is also adding the initial implementation of AvatarGroup. How would you feel about moving that into a separate PR?
| /** | ||
| * Sizes that can be used for the Avatar | ||
| */ | ||
| export type AvatarSizes = 20 | 24 | 28 | 32 | 36 | 40 | 48 | 56 | 64 | 72 | 96 | 120 | 128; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also probably want to export it from @fluentui/react-avatar/src/index and from @fluentui/react-components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make a follow up PR for the @fluentui/react-components part.
packages/react-avatar/src/stories/AvatarGroupDefault.stories.tsx
Outdated
Show resolved
Hide resolved
packages/react-avatar/src/components/AvatarGroup/AvatarGroup.strings.tsx
Show resolved
Hide resolved
* Adding initial spec * Adding more details * adding more infon * Adding almost done spec * change files * updating files to match props * Adding more information on size behavior * adding requested changes * fixing useAvatarGroup * updating api * Adding yarn.lock * updating structure of AvatarGroup and updating spec * updating package.json's dependencies * fixing typo * adding requested changes * renaming grid and stack, adding color order, adding color override example and info * removing some implementation and adding requested changes * moving react-avatar to react-components * reformatting colors table
PR details
This PR adds the following things to
AvatarGroup:Related Issue(s)
Related #22240 #22320