Allow customization of AvatarStack hover behavior and size by mperrotti · Pull Request #3466 · primer/react

mperrotti · 2023-06-30T20:17:03Z

Allows consumers to disable the behavior where the stack expands to show all avatars on hover
Allows consumers to customize the size for the avatars in the stack. This can be done two ways:
- pass a size prop to AvatarStack
- pass a size prop to the Avatars that are direct children of AvatarStack
Does not allow an AvatarStack to render it's children at different sizes. The smallest size will always be picked.

Closeshttps://github.com/github/primer/issues/2366

Screenshots

Kapture.2023-06-30.at.16.09.20.mp4

Merge checklist

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

changeset-bot · 2023-06-30T20:17:06Z

🦋 Changeset detected

Latest commit: 356bf65

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2023-06-30T20:21:13Z

size-limit report 📦

Path	Size
dist/browser.esm.js	102.31 KB (+0.69% 🔺)
dist/browser.umd.js	102.88 KB (+0.71% 🔺)

broccolinisoup

Love it! 💖 Just left a tiny comment - not a blocker.

broccolinisoup · 2023-07-03T08:52:51Z

 count?: number
 } & SxProp

+const findSmallestNumber = (numbers: number[]): number => {


Would Math.min(...numbers) work here? 🤔 Just an idea - not a blocker at all.

Great suggestion! Soooo much easier.

maximedegreve

This is great @mperrotti 🎉

One thing that's missing in this PR is support for breakpoints.

Otherwise developers will have to get around this issue using:

<Boxsx={{display: ['block','block','none']}}><AvatarStacksize={26}/></Box><Boxsx={{display: ['none','none','block']}}><AvatarStacksize={20}/></Box>

Instead of:

<AvatarStacksize={[20,20,24]}/>

Without this support it will also make working with HTML grids hard because the Box is never removed from the DOM and will create an additional grid cell.

siddharthkp · 2023-07-03T11:41:47Z


 export type AvatarStackProps = {
 alignRight?: boolean
+ disableExpand?: boolean


Personal opinion, not strongly held: disableExpand is a double negative, which can feel confusing
What do you think about expand: boolean where default is true?
<AvatarStack/><AvatarStackexpand={false}/>

It feels strange to have to explicitly pass propName={false} because boolean props are normally evaluated as false if the prop is excluded.
If we didn't set expand to true by default, then <AvatarStack /> would be the same as <AvatarStack expand={false} />

It feels strange to have to explicitly pass propName={false}
true true!
because boolean props are normally evaluated as false if the prop is excluded
not sure about that tho 😅
// default true<AvatarStack/>=<AvatarStackexpand/>=<AvatarStackexpand={true}/>// disabling default: <AvatarStackexpand={false}/>
buuuut, also happy to go with what you feel strongly about

…sable-expand

mperrotti · 2023-07-06T02:26:58Z

@broccolinisoup and @maximedegreve - I have your suggestions implemented.

This should be merge-able once I:

Update tests and stories
Fix an annoying TypeScript error when trying to pass getBreakpointDeclarations() to merge() (@broccolinisoup - maybe we can pair on this?)
Other misc code cleanup
Comment any complicated code I couldn't simplify

maximedegreve · 2023-07-06T11:55:56Z

@mperrotti Oh no!

mperrotti · 2023-07-06T12:42:50Z

Haha thanks for sharing that @maximedegreve - I'll be fixing that while I make my other changes today.

…sable-expand

maximedegreve · 2023-07-06T21:52:45Z

LGTM 🚀💪

dusave · 2023-07-06T22:53:50Z

Broader question, not a blocker for this PR: Are we adding any sort of prefers-reduced-motion rule in the animation definitions of components to disable them users automatically? This could be supplemented with the explicit prop (the consumer might want it disabled for all users), but for accessibility reasons, adding the css check could make it much easier to work with.

broccolinisoup

Looks great! Just left a comment about the type stuff but feel free to ignore if it is not helpful.

🚀 🚀 🚀

broccolinisoup · 2023-07-07T02:12:53Z

 ) {
- return <StyledAvatar ref={ref} alt={alt} size={size} square={square} {...rest} />
+ const avatarSx = isResponsiveValue(size)
+ ? merge(


Sorry I missed your ping about the type stuff.
I think we could also do
merge<BetterSystemStyleObject>(...)
instead of typing each input as?
Also totally optional maybe for readability
constavatarSx=isResponsiveValue(size) ? getBreakpointDeclarations(size,'--avatar-size'askeyofReact.CSSProperties,value=>`${value||DEFAULT_AVATAR_SIZE}px`,) : {'--avatar-size': `${size}px`}
and then in the StyledAvatar
sx={merge<BetterSystemStyleObject>(avatarSx, sxProp)}
Just a suggestion - however you prefer.

I think I remember there being an performance issue with passing merge() directly to to sx, so I'm going to keep it as-is. Definitely going to use a generic instead of as though. Great suggestion!

Ohh I didn't know about the performance impact on using merge in sx! Thanks for letting me know!

mperrotti · 2023-07-07T13:37:50Z

@dusave

Are we adding any sort of prefers-reduced-motion rule in the animation definitions of components to disable them users automatically?

We do use prefers-reduced-motion rules in some components, but not this one. I'm going to check with a11y design to see if it makes sense for this small transition.

mperrotti · 2023-07-07T15:18:52Z

@dusave - I checked with a11y, and it looks like this is safe to keep enabled even when the user prefers reduced motion.

…sable-expand

…ct into mp/avatarstack-disable-expand

mperrotti added 3 commits June 29, 2023 15:32

adds a prop to prevent avatars from spreading on hover
af5de05

enables AvatarStack to render any size avatar
0f1a410

updates tests and stories
b56fb75

mperrotti requested review from a team and langermank June 30, 2023 20:17

mperrottiand others added 2 commits June 30, 2023 20:18

Update generated/components.json
51c6798

Create eighty-islands-join.md
dc66fd8

Merge branch 'main' into mp/avatarstack-disable-expand
aaa91bc

github-actionsBot temporarily deployed to storybook-preview-3466 June 30, 2023 20:24 Inactive

github-actionsBot temporarily deployed to storybook-preview-3466 June 30, 2023 20:25 Inactive

mperrotti temporarily deployed to github-pages June 30, 2023 20:27 — with GitHub Actions Inactive

github-actionsBot temporarily deployed to storybook-preview-3466 June 30, 2023 20:27 Inactive

github-actionsBot temporarily deployed to storybook-preview-3466 June 30, 2023 20:28 Inactive

broccolinisoup approved these changes Jul 3, 2023
View reviewed changes

maximedegreve approved these changes Jul 3, 2023
View reviewed changes

maximedegreve reviewed Jul 3, 2023
View reviewed changes

siddharthkp reviewed Jul 3, 2023
View reviewed changes

mperrotti added 2 commits July 5, 2023 18:55

Merge branch 'main' of github.com:primer/react into mp/avatarstack-di…
e9ac0b5
…sable-expand

replace custom function to find smallest number with Math.min
1c6d74c

mperrotti temporarily deployed to github-pages July 5, 2023 23:02 — with GitHub Actions Inactive

github-actionsBot temporarily deployed to storybook-preview-3466 July 5, 2023 23:03 Inactive

makes Avatar and AvatarStack 'size' prop responsive
b791f1c

mperrotti temporarily deployed to github-pages July 6, 2023 02:34 — with GitHub Actions Inactive

github-actionsBot temporarily deployed to storybook-preview-3466 July 6, 2023 02:34 Inactive

updates stories and tests, fixes bugs
538585e

mperrotti added 2 commits July 6, 2023 16:27

updates snapshots after updating dependencies
b1aec60

Merge branch 'main' of github.com:primer/react into mp/avatarstack-di…
03e0f7d
…sable-expand

mperrotti temporarily deployed to github-pages July 6, 2023 20:41 — with GitHub Actions Inactive

github-actionsBot temporarily deployed to storybook-preview-3466 July 6, 2023 20:41 Inactive

mperrotti added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Jul 6, 2023

test(vrt): update snapshots
ffd7719

github-actionsBot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Jul 6, 2023

primerBot temporarily deployed to github-pages July 6, 2023 21:13 Inactive

github-actionsBot temporarily deployed to storybook-preview-3466 July 6, 2023 21:14 Inactive

broccolinisoup approved these changes Jul 7, 2023
View reviewed changes

mperrotti added 3 commits July 7, 2023 11:24

typescript improvements
efbea09

Merge branch 'main' of github.com:primer/react into mp/avatarstack-di…
0ff5a97
…sable-expand

Merge branch 'mp/avatarstack-disable-expand' of github.com:primer/rea…
1fb8e97
…ct into mp/avatarstack-disable-expand

mperrotti enabled auto-merge July 7, 2023 15:25

mperrotti temporarily deployed to github-pages July 7, 2023 15:33 — with GitHub Actions Inactive

github-actionsBot temporarily deployed to storybook-preview-3466 July 7, 2023 15:34 Inactive

convert clasnames to clsx
356bf65

mperrotti deployed to github-pages July 7, 2023 15:55 — with GitHub Actions View deployment

github-actionsBottemporarily deployed to storybook-preview-3466 July 7, 2023 15:55 Inactive

mperrotti added this pull request to the merge queue Jul 7, 2023

Merged via the queue into main with commit 11b54acJul 7, 2023

mperrotti deleted the mp/avatarstack-disable-expand branch July 7, 2023 16:13

This was referenced Jul 7, 2023
Release tracking #3493
Merged
Release tracking (next major) #3147
Closed

maximedegreve assigned mperrotti Aug 14, 2023

Uh oh!

Conversation

mperrotti commented Jun 30, 2023

Screenshots

Merge checklist

Uh oh!

changeset-botBot commented Jun 30, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actionsBot commented Jun 30, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

broccolinisoup left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

broccolinisoupJul 3, 2023

Choose a reason for hiding this comment

Uh oh!

mperrottiJul 5, 2023

Choose a reason for hiding this comment

Uh oh!

maximedegreve left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddharthkpJul 3, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mperrottiJul 5, 2023

Choose a reason for hiding this comment

Uh oh!

siddharthkpJul 6, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mperrotti commented Jul 6, 2023

Uh oh!

maximedegreve commented Jul 6, 2023

Uh oh!

mperrotti commented Jul 6, 2023

Uh oh!

maximedegreve commented Jul 6, 2023

Uh oh!

dusave commented Jul 6, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

broccolinisoup left a comment

Choose a reason for hiding this comment

Uh oh!

broccolinisoupJul 7, 2023

Choose a reason for hiding this comment

Uh oh!

mperrottiJul 7, 2023

Choose a reason for hiding this comment

Uh oh!

broccolinisoupJul 8, 2023

Choose a reason for hiding this comment

Uh oh!

mperrotti commented Jul 7, 2023

Uh oh!

mperrotti commented Jul 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

changeset-botBot commented Jun 30, 2023•
edited
Loading

github-actionsBot commented Jun 30, 2023•
edited
Loading

broccolinisoup left a comment •
edited
Loading

maximedegreve left a comment •
edited
Loading

siddharthkpJul 3, 2023•
edited
Loading

siddharthkpJul 6, 2023•
edited
Loading

dusave commented Jul 6, 2023•
edited
Loading