Skip to content

Conversation

@sopranopillow
Copy link
Contributor

@sopranopillow sopranopillow commented Jun 21, 2022

Current Behavior

AvatarGroup warns when an Avatar component is used inside an AvatarGroup.

When given a fragment containing the AvatarGroupItems such as:

<AvatarGroup>
  <>
    <AvatarGroupItem />
    <AvatarGroupItem />
    <AvatarGroupItem />
    <AvatarGroupItem />
    <AvatarGroupItem />
  </>
</AvatarGroup>

AvatarGroup does not handle the items correctly and overflow is not handled correctly.

New Behavior

As mentioned by Shift in this comment, AvatarGroup's warning is breaking the composition pattern. To avoid this, the warning has been removed.

AvatarGroup now checks if the children contains a fragment and gets its contents.
Note: this is a temporary fix of a bigger problem, to follow up on this issue, see #23666.

@sopranopillow sopranopillow requested a review from a team as a code owner June 21, 2022 19:16
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 21, 2022

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 292b883:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 21, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1698 1665 5000
Button mount 1222 1197 5000
FluentProvider mount 2264 2267 5000
FluentProviderWithTheme mount 870 914 10
FluentProviderWithTheme virtual-rerender 864 867 10
FluentProviderWithTheme virtual-rerender-with-unmount 869 895 10
MakeStyles mount 2451 2426 50000

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 21, 2022

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-avatar
Avatar
46.031 kB
13.438 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
185.394 kB
51.302 kB
react-components
react-components: FluentProvider & webLightTheme
31.16 kB
10.215 kB
🤖 This report was generated against 15fe9ac48a636c964e6ad1cd36fa5a6d4d5d52af

@size-auditor
Copy link

size-auditor bot commented Jun 21, 2022

Asset size changes

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

Baseline commit: 15fe9ac48a636c964e6ad1cd36fa5a6d4d5d52af (build)

@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react-northstar)

⚠️ 2 potential perf regressions detected

Potential regressions comparing to master

Scenario Current PR Ticks Baseline Ticks Ratio Regression Analysis
AttachmentSlotsPerf.default 927 879 1.05:1 analysis
AlertMinimalPerf.default 224 220 1.02:1 analysis
Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
BoxMinimalPerf.default 282 243 1.16:1
TableMinimalPerf.default 337 291 1.16:1
ButtonMinimalPerf.default 135 120 1.13:1
LoaderMinimalPerf.default 574 506 1.13:1
ReactionMinimalPerf.default 302 267 1.13:1
ListWith60ListItems.default 517 477 1.08:1
RefMinimalPerf.default 210 194 1.08:1
AccordionMinimalPerf.default 120 112 1.07:1
AttachmentMinimalPerf.default 124 116 1.07:1
AvatarMinimalPerf.default 162 151 1.07:1
CardMinimalPerf.default 458 428 1.07:1
FormMinimalPerf.default 341 323 1.06:1
GridMinimalPerf.default 288 272 1.06:1
ImageMinimalPerf.default 307 290 1.06:1
ButtonSlotsPerf.default 466 443 1.05:1
MenuButtonMinimalPerf.default 1437 1369 1.05:1
SkeletonMinimalPerf.default 289 275 1.05:1
LabelMinimalPerf.default 321 310 1.04:1
PopupMinimalPerf.default 526 505 1.04:1
PortalMinimalPerf.default 143 137 1.04:1
SliderMinimalPerf.default 1413 1364 1.04:1
ChatDuplicateMessagesPerf.default 242 235 1.03:1
ItemLayoutMinimalPerf.default 987 954 1.03:1
CarouselMinimalPerf.default 399 392 1.02:1
ChatMinimalPerf.default 626 612 1.02:1
ChatWithPopoverPerf.default 325 318 1.02:1
ListCommonPerf.default 540 527 1.02:1
ListNestedPerf.default 455 445 1.02:1
ProviderMinimalPerf.default 337 331 1.02:1
ToolbarMinimalPerf.default 799 785 1.02:1
TreeMinimalPerf.default 675 664 1.02:1
TreeWith60ListItems.default 130 127 1.02:1
ButtonOverridesMissPerf.default 1280 1269 1.01:1
CheckboxMinimalPerf.default 2311 2287 1.01:1
DialogMinimalPerf.default 641 634 1.01:1
DividerMinimalPerf.default 291 287 1.01:1
DropdownMinimalPerf.default 2646 2625 1.01:1
EmbedMinimalPerf.default 3457 3434 1.01:1
HeaderMinimalPerf.default 299 295 1.01:1
ProviderMergeThemesPerf.default 1055 1048 1.01:1
RadioGroupMinimalPerf.default 369 365 1.01:1
SegmentMinimalPerf.default 285 282 1.01:1
StatusMinimalPerf.default 557 553 1.01:1
TableManyItemsPerf.default 1602 1592 1.01:1
CustomToolbarPrototype.default 2317 2291 1.01:1
DatepickerMinimalPerf.default 4773 4762 1:1
HeaderSlotsPerf.default 620 618 1:1
MenuMinimalPerf.default 708 707 1:1
VideoMinimalPerf.default 551 551 1:1
AnimationMinimalPerf.default 451 455 0.99:1
ListMinimalPerf.default 426 434 0.98:1
TextMinimalPerf.default 283 290 0.98:1
TooltipMinimalPerf.default 926 946 0.98:1
DropdownManyItemsPerf.default 548 563 0.97:1
InputMinimalPerf.default 1012 1042 0.97:1
FlexMinimalPerf.default 237 246 0.96:1
LayoutMinimalPerf.default 284 295 0.96:1
RosterPerf.default 919 953 0.96:1
SplitButtonMinimalPerf.default 3422 3642 0.94:1
IconMinimalPerf.default 455 486 0.94:1
TextAreaMinimalPerf.default 365 390 0.94:1

@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 757 793 5000
Breadcrumb mount 2368 2373 1000
Checkbox mount 1305 1264 5000
CheckboxBase mount 1069 1104 5000
ChoiceGroup mount 4165 4126 5000
ComboBox mount 838 868 1000
CommandBar mount 9063 9038 1000
ContextualMenu mount 10103 10006 1000
DefaultButton mount 971 965 5000
DetailsRow mount 3297 3281 5000
DetailsRowFast mount 3334 3307 5000
DetailsRowNoStyles mount 3212 3130 5000
Dialog mount 2473 2474 1000
DocumentCardTitle mount 152 150 1000
Dropdown mount 3139 2900 5000
FocusTrapZone mount 1667 1686 5000
FocusZone mount 1556 1600 5000
IconButton mount 1519 1504 5000
Label mount 299 315 5000
Layer mount 2520 2544 5000
Link mount 398 405 5000
MenuButton mount 1269 1253 5000
MessageBar mount 1791 1896 5000
Nav mount 2841 2858 1000
OverflowSet mount 950 939 5000
Panel mount 1884 1908 1000
Persona mount 872 852 1000
Pivot mount 1253 1229 1000
PrimaryButton mount 1126 1118 5000
Rating mount 6713 6704 5000
SearchBox mount 1108 1120 5000
Shimmer mount 2098 2147 5000
Slider mount 1689 1664 5000
SpinButton mount 4312 4354 5000
Spinner mount 381 371 5000
SplitButton mount 2737 2713 5000
Stack mount 443 453 5000
StackWithIntrinsicChildren mount 2023 2009 5000
StackWithTextChildren mount 4537 4513 5000
SwatchColorPicker mount 10053 10120 5000
TagPicker mount 2349 2347 5000
TeachingBubble mount 80149 80026 5000
Text mount 355 363 5000
TextField mount 1208 1245 5000
ThemeProvider mount 1023 1024 5000
ThemeProvider virtual-rerender 549 550 5000
ThemeProvider virtual-rerender-with-unmount 1617 1618 5000
Toggle mount 681 696 5000
buttonNative mount 111 108 5000

@ling1726
Copy link
Contributor

to follow up on this issue, see

sorry what issue ? 😅

@sopranopillow
Copy link
Contributor Author

to follow up on this issue, see

sorry what issue ? 😅

sorry I'm still writing it and forgot to remove that 😅

@sopranopillow sopranopillow changed the title fix(react-avatar): Remove warnings from AvatarGroup and AvatarGroupItem and fix problem with fragments in AvatarGroup fix(react-avatar): Remove warning from AvatarGroup and fix problem with fragments in AvatarGroup Jun 22, 2022
Comment on lines +19 to +21
const childrenArray = React.Children.toArray(
React.isValidElement(children) && children.type === React.Fragment ? children.props.children : children,
);
Copy link
Member

@layershifter layershifter Jun 23, 2022

Choose a reason for hiding this comment

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

Don't want to make it worse, but the tree could be nested...

<AvatarGroup>
  <>
    <AvatarGroupItem />
    <AvatarGroupItem />
    <>
      <AvatarGroupItem />
      <AvatarGroupItem />
      <AvatarGroupItem />
    </>
  </>
</AvatarGroup>

TBH Not sure if it worth fixing currently, I would rather fix it properly 🐱

@sopranopillow sopranopillow deleted the avatargroup-dom branch June 29, 2022 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

4 participants