Material-ui: [AvatarGroup] Move to the core

[Studio384]

• [x] The issue is present in the latest release.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Currently, the only indication that AvatarGroup is a lab-component is its import statement in the example that is hidden by default. This should be mentioned in the text above the example.

The 4.8.0 release notes also don't mention this and even list AvatarGroup as part of core instead of lab.

I'd argue it would be even better to just split this component to its own page that is listed under "Lab" in the menu, and merge that back into the Avatar-page once the component is stable.

[oliviertassinari]

@Studio384 From my perspective, we could move the component as is in the core. In the pull request, we have explored a couple of enhancement, none required a breaking change:

• add a spacing prop
• add a max prop to have an automatic +x behavior

@mbrookes What do you think?

[mbrookes]

Given how simple the internals are, I have no objection to moving this to the core, and adding features later based on demand. Neither of the suggested features is technically challenging, so unlikely to cause issues.

[oliviertassinari]

@Studio384 I think that we can wait a couple of weeks before taking on this task, it would be great for v4.9.0 :). Feel free to take the lead of it 💪.

[Studio384]

@oliviertassinari In the meantime, shouldn't the line about AvatarGroup in the 4.8.0 release notes be moved from the release notes of the core package to the notes for lab?

[oliviertassinari]

Fixed, thanks

[afzalsayed96]

Hey, I have an abstraction around Avatar like this

function UserAvatar({ user, ...other }) {
  const classes = useStyles()

  return (
    <Avatar
      alt={user.name}
      className={classes.avatar}
      src={user.avatar}
      title={user.name}
      {...other}
    >
      {user.initials}
    </Avatar>
  )
}

Now if I use this component inside AvatarGroup the styles from my provided className won't apply to Avatar.

The reason being that AvatarGroup is looking for className on my custom UserAvatar component and not it's child Avatar here.
https://github.com/mui-org/material-ui/blob/a07f18029a5532de310f234392c18664817f0dce/packages/material-ui-lab/src/AvatarGroup/AvatarGroup.js#L41

Do you suggest any workaround for this issue? How does this work correctly with Tooltip? I'll be happy to make a PR if required.

Thanks

Edit

Never mind. This seems to work:

-function UserAvatar({ user, ...other }) {
+function UserAvatar({ user, className, ...other }) {
  const classes = useStyles()

  return (
    <Avatar
      alt={user.name}
-     className={classes.avatar}
+     className={clsx(className,classes.avatar)}
      src={user.avatar}
      title={user.name}
      {...other}
    >
      {user.initials}
    </Avatar>
  )
}

Cheers

[GFynbo]

Hi all,

I'd like to take a stab at this issue. This is my first time contributing and any advice or recommendations would be greatly appreciated. From what I can see, the following needs to be done:

• Move AvatarGroup from Lab --> Core
• Update docs to reflect AvatarGroup in Core as well as the Avatar docs with correct import
• Add a spacing prop
• Add a max prop to have automatic +x behavior

And for max +x behavior are you referring to defining a limit of avatar's shown and the remainder as the +x as shown in the docs @oliviertassinari ? If so should that be in these changes or as a separate feature?

Correct me if I'm wrong or have any misunderstandings.

Thanks!

[oliviertassinari]

This sounds to me that it should be the subject of 3 different pull requests. 1. move to core 2. add spacing prop 3. add max prop. Otherwise, great, happy to see you around :).

[mbrookes]

I'd suggest that the move to core should come last.

[GFynbo]

What should the spacing prop be defined by? Just raw values ranging from some number such as 2 to -12 or something?

Or is there a best practice that I'm not aware of?

[oliviertassinari]

Great question. My initial intuition was to say a to use a multiple of theme.spacing unit (1.), but in this context, I wonder if a px unit wouldn't be better (2.). For instance, let's say a user customize theme.spacing, should it impact the AvatarGroup?

[GFynbo]

Yeah that was my exact concern too. I think because the existing functionality is just an explicitly set pixel value and to change it requires using css we can extend that and allow those preset spacing options or they can continue to change via css. It feels to me that it should be unaffected by theme.spacing, do you agree?

I was thinking 3 preset options of

• 0: -4px
• 1: -8px
• 2: -12px

Or something along those lines.

[GFynbo]

I went with 5 spacings ranging from -4px to -20px in 4px increments. I've got the changes up in a feature branch and made changes to the API documentation too, but I'm not sure what/where any testing should be included. @oliviertassinari

[oliviertassinari]

@GFynbo Right, there is this third option (3.): to accept an enum, like small, medium, large values.

[GFynbo]

@oliviertassinari Yeah maybe that's the best way forward. I've changed it to reflect that with options for small, medium and large with corresponding -16px, -8px, and -4px. I'll create the first PR now.

[GFynbo]

Spacing prop was merged into master with #19761, working on max prop now.

[dbarabashdev]

@oliviertassinari is it ready for moving to the core?

[GFynbo]

@oliviertassinari Do we want the max prop to have be the default functionality? For example if I pass an AvatarGroup with 6 Avatar children, should the default behavior be to show 3 and have +3 at the end or to show all 6 and only have +x functionality when explicitly defined?

[oliviertassinari]

@GFynbo Great question, I think that we can consider a default value for the max prop.
Regarding the behavior, let's say max=5, if we provide 8 children, we would display 5 avatars and +3. Does it match your intuitive expectations?

[GFynbo]

@oliviertassinari Certainly makes sense to me! I'll work on that.

[Studio384]

Should the max prop show the max number of user avatars, or just avatars tho? Do we expect max=5 to give 5 avatars and a +-avatar, or 4 avatars and a 5th avatar with the +-avatar in case it is more than 5.

[GFynbo]

I think reasonable behavior is to show the number defined with an additional +x avatar shown after for a total of 6 avatars displayed. It seems fairly logical to have the max shown prop show that number of avatars with the remaining in an additional +x prop.

[GFynbo]

PR #19853 deals with max prop

[mbrookes]

It seems that this was missed from #20012, so I've added it.

[HJain13]

Hey @oliviertassinari @mbrookes can I take a stab at moving AvatarGroup to core?

[mbrookes]

@HJain13 Great! You can follow the other lab to core PRs linked from #20012 for an idea of what needs to be done.