Skip to content

Conversation

@sopranopillow
Copy link
Contributor

@sopranopillow sopranopillow commented Apr 29, 2022

PR Details

This PR adds the following:

Related Issue(s)

Related #22240 #22320

Fixes #22883

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 29, 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 6b38759:

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

@fabricteam
Copy link
Collaborator

fabricteam commented Apr 29, 2022

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-avatar
Avatar
45.538 kB
13.298 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
184.781 kB
51.176 kB
react-components
react-components: FluentProvider & webLightTheme
33.988 kB
11.108 kB
🤖 This report was generated against 418c78ea83242d3354250a8394bfa0ef0f78cadb

@fabricteam
Copy link
Collaborator

fabricteam commented Apr 29, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 883 901 5000
Button mount 582 551 5000
FluentProvider mount 1896 2022 5000
FluentProviderWithTheme mount 268 234 10
FluentProviderWithTheme virtual-rerender 230 242 10
FluentProviderWithTheme virtual-rerender-with-unmount 287 338 10
MakeStyles mount 1675 1651 50000

@size-auditor
Copy link

size-auditor bot commented Apr 29, 2022

Asset size changes

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

Baseline commit: 418c78ea83242d3354250a8394bfa0ef0f78cadb (build)

@sopranopillow sopranopillow requested a review from a team as a code owner May 4, 2022 15:24
@sopranopillow sopranopillow requested a review from micahgodbolt May 4, 2022 17:09
@sopranopillow sopranopillow requested a review from a team as a code owner May 9, 2022 22:12
@sopranopillow sopranopillow removed the request for review from a team May 9, 2022 22:25
@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
ImageMinimalPerf.default 379 351 1.08:1
ListMinimalPerf.default 532 499 1.07:1
PortalMinimalPerf.default 176 165 1.07:1
HeaderMinimalPerf.default 367 345 1.06:1
TreeWith60ListItems.default 169 159 1.06:1
AttachmentMinimalPerf.default 154 147 1.05:1
AvatarMinimalPerf.default 197 187 1.05:1
BoxMinimalPerf.default 347 332 1.05:1
FlexMinimalPerf.default 282 269 1.05:1
GridMinimalPerf.default 339 323 1.05:1
ListWith60ListItems.default 655 624 1.05:1
PopupMinimalPerf.default 637 608 1.05:1
VideoMinimalPerf.default 669 640 1.05:1
ListCommonPerf.default 639 612 1.04:1
MenuButtonMinimalPerf.default 1743 1681 1.04:1
TableMinimalPerf.default 398 383 1.04:1
TreeMinimalPerf.default 824 790 1.04:1
ButtonMinimalPerf.default 162 158 1.03:1
ButtonSlotsPerf.default 544 526 1.03:1
LayoutMinimalPerf.default 357 346 1.03:1
RadioGroupMinimalPerf.default 448 435 1.03:1
SliderMinimalPerf.default 1716 1672 1.03:1
DividerMinimalPerf.default 342 334 1.02:1
DropdownMinimalPerf.default 3084 3016 1.02:1
ItemLayoutMinimalPerf.default 1186 1168 1.02:1
AccordionMinimalPerf.default 139 137 1.01:1
CarouselMinimalPerf.default 474 468 1.01:1
ChatDuplicateMessagesPerf.default 286 284 1.01:1
DialogMinimalPerf.default 777 767 1.01:1
EmbedMinimalPerf.default 4151 4114 1.01:1
HeaderSlotsPerf.default 757 747 1.01:1
InputMinimalPerf.default 1307 1294 1.01:1
LabelMinimalPerf.default 376 372 1.01:1
RosterPerf.default 1121 1110 1.01:1
SegmentMinimalPerf.default 341 336 1.01:1
TableManyItemsPerf.default 1930 1905 1.01:1
TextAreaMinimalPerf.default 487 481 1.01:1
ToolbarMinimalPerf.default 951 939 1.01:1
AlertMinimalPerf.default 262 262 1:1
AnimationMinimalPerf.default 547 547 1:1
ButtonOverridesMissPerf.default 1485 1482 1:1
FormMinimalPerf.default 400 401 1:1
ListNestedPerf.default 537 535 1:1
MenuMinimalPerf.default 852 850 1:1
ProviderMergeThemesPerf.default 1262 1262 1:1
ProviderMinimalPerf.default 394 394 1:1
TextMinimalPerf.default 334 333 1:1
TooltipMinimalPerf.default 1050 1051 1:1
CardMinimalPerf.default 540 544 0.99:1
CheckboxMinimalPerf.default 2684 2709 0.99:1
SplitButtonMinimalPerf.default 4348 4399 0.99:1
CustomToolbarPrototype.default 2720 2756 0.99:1
ChatWithPopoverPerf.default 372 378 0.98:1
DatepickerMinimalPerf.default 5724 5813 0.98:1
DropdownManyItemsPerf.default 660 673 0.98:1
ReactionMinimalPerf.default 365 371 0.98:1
RefMinimalPerf.default 235 240 0.98:1
SkeletonMinimalPerf.default 329 336 0.98:1
StatusMinimalPerf.default 658 671 0.98:1
IconMinimalPerf.default 589 598 0.98:1
AttachmentSlotsPerf.default 1070 1098 0.97:1
LoaderMinimalPerf.default 687 705 0.97:1
ChatMinimalPerf.default 719 749 0.96:1

@sopranopillow sopranopillow removed the request for review from a team June 2, 2022 18:58
@sopranopillow sopranopillow requested a review from behowell June 3, 2022 01:51
Copy link
Contributor

@behowell behowell left a comment

Choose a reason for hiding this comment

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

Some more feedback -- thanks for working on this!

There are plenty of comments that could be addressed in a future PR if you'd like to get this one in sooner than later. However, I'd ask that you log an issue to track that work so it doesn't get forgotten. Thanks!

<slots.root {...slotProps.root}>
{state.root.children}
{state.hasOverflow && (
<Popover trapFocus size="small">
Copy link
Contributor

Choose a reason for hiding this comment

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

We might also need a slot for this Popover so it can be configured by the user (fine to do in a separate PR).

Copy link
Contributor Author

@sopranopillow sopranopillow Jun 6, 2022

Choose a reason for hiding this comment

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

Since Popover is not rendered as an element, it cannot be made into a slot.

@sopranopillow sopranopillow requested a review from behowell June 6, 2022 17:45
@sopranopillow sopranopillow mentioned this pull request Jun 6, 2022
7 tasks
@sopranopillow sopranopillow merged commit 8e8a1f0 into microsoft:master Jun 6, 2022
@sopranopillow sopranopillow deleted the avatargroup-implemenation branch June 6, 2022 22:58
marwan38 pushed a commit to marwan38/fluentui that referenced this pull request Jun 13, 2022
…t#22736)

* Adding initial implementation of AvatarGroup

* change files

* updating popover surface items to use ul and li, updating slots to be nonnullabel

* updating types and package.json

* updating api

* adding missing dependency

* removing token replacement in strings, removing commons from types, and adding tabindex to popoverSurface items

* reverting yarn.lock

* fixing dependencies

* updating types and renaming variables

* removing strings prop

* adding requested changes

* removing useAvatarGroup.ts

* adding context to Avatar and AvatarGroup

* adding single initial for pie layout

* Adding requested changes

* updating snapshots

* updating stories

* removing context form Avatar

* updating AvatarGroup

* removing v9 button from AvtarGroup

* removing styling for trigger

* updating api

* Adding requested changes

* updating stories and snapshots

* adding requested changes
Comment on lines +25 to +39
if (layout === 'pie') {
rootChildren = childrenArray.slice(0, 3);
overflowChildren = childrenArray;
} else if (childrenArray.length > maxAvatars) {
const numOfAvatarsToHide = childrenArray.length - maxAvatars + 1;

rootChildren = childrenArray.slice(numOfAvatarsToHide);
overflowChildren = childrenArray.slice(0, numOfAvatarsToHide);

if (overflowIndicator === 'icon') {
overflowButtonChildren = <MoreHorizontalRegular />;
} else {
overflowButtonChildren = numOfAvatarsToHide > 99 ? '99+' : `+${numOfAvatarsToHide}`;
}
}
Copy link
Member

@layershifter layershifter Jun 21, 2022

Choose a reason for hiding this comment

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

Children iteration was proven as the pattern that breaks composition 😥

There are some examples below:

function App() {
  return (
    <AvatarGroup>
      <>
        <AvatarGroupItem />
        <AvatarGroupItem />
      </>
    </AvatarGroup>
  );
}
function AlwaysPresentAvatars() {
  return (
    <>
      <AvatarGroupItem />
      <AvatarGroupItem />
    </>
  );
}

function App() {
  return (
    <AvatarGroup>
      <AlwaysPresentAvatars />
    </AvatarGroup>
  );
}

Both snippets will render the same DOM markup, but they will not be properly handled by AvatarGroup component.

I think that we need to figure out a better pattern to handle these scenarios in AvatarGroup as in current state can lead to unexpected problems and #23498 does not prevent them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: AvatarGroup API does not allow correct plural translation

6 participants