[AvatarGroup] Allow specifying total number of avatars #29898
Conversation
mbrookes
changed the title
mbrookes
added
component: avatar
oliviertassinari
changed the title
oliviertassinari
changed the title
|
@eduardomcv Can you check my comment before we make a decision about this new prop? #29649 (comment) |
| const totalAvatars = total === undefined || total < 0 ? children.length : total; | ||
| const clampedChildren = totalAvatars < children.length ? totalAvatars : children.length; | ||
|
|
||
| const shownAvatars = | ||
| totalAvatars <= clampedMax || clampedChildren < clampedMax ? clampedChildren : clampedMax - 1; | ||
|
|
||
| const extraAvatars = totalAvatars - shownAvatars; |
There was a problem hiding this comment.
Can you remove the clampedChildren? it makes the code very hard to understand and we need to document it.
| const totalAvatars = total === undefined || total < 0 ? children.length : total; | |
| const clampedChildren = totalAvatars < children.length ? totalAvatars : children.length; | |
| const shownAvatars = | |
| totalAvatars <= clampedMax || clampedChildren < clampedMax ? clampedChildren : clampedMax - 1; | |
| const extraAvatars = totalAvatars - shownAvatars; | |
| const totalAvatars = total || children.length; | |
| const extraAvatars = totalAvatars > clampedMax ? totalAvatars - clampedMax + 1 : 0; |
There was a problem hiding this comment.
@siriwatknp I removed clampedChildren and refactored the code into something closer to your suggestion.
A problem arises when there are less child avatars than the clampedMax. In summary, we need to take into account children.length when calculating extraAvatars. Using the example from the 'should respect total' test to illustrate the problem:
<AvatarGroup total={10}>
<Avatar src="/fake.png" />
<Avatar src="/fake.png" />
<Avatar src="/fake.png" />
</AvatarGroup>In this example, there will be 10 totalAvatars, and since max was not specified, clampedMax will be 5. Applying the suggested calculation above, extraAvatars will equal 6 (10 - 5 + 1). However, this is incorrect, because given the fact that there are only 3 child avatars available, the component will render 3 avatars and a "+6".
There was a problem hiding this comment.
I think the logic should be simple as I suggested. cc @mnajdova
|
@eduardomcv Sorry that I change your code because I added more tests and they failed, so I have to adjust the implementation to make it works. Can you review it again? (if you have a better way, feel free to change because I think we have a good amount of tests that should cover most cases) Also added one demo for the |
| it('should respect total and clamp down shown avatars', () => { | ||
| const { container } = render( | ||
| <AvatarGroup total={1}> | ||
| <Avatar src="/fake.png" /> | ||
| <Avatar src="/fake.png" /> | ||
| </AvatarGroup>, | ||
| ); | ||
| expect(container.querySelectorAll('.MuiAvatar-root').length).to.equal(1); | ||
| expect(container.querySelectorAll('img').length).to.equal(1); | ||
| expect(container.textContent).to.equal(''); | ||
| }); |
There was a problem hiding this comment.
I don't understand this test. There are two avatars, but the total number is one?
There was a problem hiding this comment.
It tests that the total prop should be used instead of children.length
Co-authored-by: Marija Najdova <mnajdova@gmail.com>
Closes #29649
Allow the developer to specify a
totalnumber of avatars. If atotalis provided, allAvatarGroupcalculations will be based on this number, rather than relying solely onchildren.length.This is useful in use cases where the avatars are based on paginated lists and the server exposes the total count. There shouldn't be a need to request the whole list from the server (or to generate a list with that length) just to fulfil the requirement of the
AvatarGroup.