Fix AvatarStack opacity for children without sx prop

[talune]

What this changes

Resolveshttps://github.com/github/memex/issues/6311

This change adjusts how style rules are defined for .pc-AvatarItem so that children of AvatarStack display correctly regardless of if the child element has an sx prop.

Previously, opacity was defined in AvatarStackWrapper, but z-index was passed to the child's sx prop. If the child element didn't have the sx prop, then avatars were stacked in reverse order, with the least opaque on top, as can be seen in the storybook example here.

To resolve, I removed the z-index mapping from transformChildren, and instead defined the appropriate z-index explicitly in the same place as opacity -- in AvatarStackWrapper.

Screenshots

Before:

After
https://user-images.githubusercontent.com/6740550/146618418-8a415298-666d-4ca4-a7d9-a1aa358701f5.mov

Merge checklist

• Added/updated tests
• Added/updated documentation
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

[changeset-bot]

🦋 Changeset detected

Latest commit: abcc056

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	Patch

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]

size-limit report 📦

Path	Size
dist/browser.esm.js	58.91 KB (-0.07% 🔽)
dist/browser.umd.js	59.24 KB (-0.06% 🔽)

[rezrah]

@talune 👋 - Thanks for putting up this PR - looks great. Wanted to check something first. The storybook example appears to be incorrect, as the correct usage for AvatarStack is here: https://primer.style/react/AvatarStack

Converting the img in your original issue example to a nested Avatarshould prevent this opacity issue from occurring. I think. Have you tried this approach instead?

[talune]

Thanks, @rezrah! Yes, you're correct that the storybook example looks off because the child elements are <img>s instead of <Avatar>s.

In my team's case, we were correctly using <Avatar> child elements, but we were inadvertently overriding the sx prop that AvatarStack maps to child elements here. We corrected our bug by augmenting the sx prop instead of replacing it.

I noticed the storybook bug while working on the above resolution, and decided to take a closer look. The storybook bug has the same root issue: <img>s don't support an sx prop. This means they don't receive the z-index rule, However, they do receive the opacity rule which isn't defined by the child sx prop.

I think it's a bit confusing to have the opacity and z-index rules defined in these two different ways, and it seemed pretty easy to resolve that by moving the z-index definition into the same block as the opacity rule.

You're absolutely right that the styles render correctly if one uses <Avatar> children without overriding the sx prop. But my hope with this change is to prevent future users from running into the same bug by simplifying how the styles are applied. However, no worries if this isn't the approach the team prefers!

[rezrah]

@talune - thanks for the background on this issue. I saw your fix here, glad it's been resolved.

We encourage using Avatar over a native img inside AvatarStack, so we'll fix up the Story so that it doesn't confuse others in future. Apologies for any confusion that caused you too.

With regards to the PR itself, I've tested it for visual regression too and it seems okay 👍 . Co-locating the styles seems sensible, but I can't reproduce the bug around Avatar sx props being overridden, as it merges prop styles with z-index fine for me 🤔 . I do like how this makes the code and design logic easier to reason about though, and will therefore help us move this component to beta faster, so thanks for this contribution.

Edit: PR was missing a changeset, which is required to merge and trigger a new npm release. I've added one for you just now, so please go ahead and merge this PR when you're happy to proceed.

[talune]

Thank you for the extra context and for adding the changeset, @rezrah! I was prompted to first merge in main & I will merge this PR once checks re-pass.