feat(AvatarStack): Convert AvatarStack to CSS modules behind team feature flag #5299
Conversation
Co-authored-by: Josh Black <[email protected]>
🦋 Changeset detectedLatest commit: f7f982a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
github-actions
bot
added
the
integration-tests: recommended
jonrohan
temporarily deployed
to
github-pages
GitHub Actions
Inactive
jonrohan
temporarily deployed
to
github-pages
GitHub Actions
Inactive
jonrohan
temporarily deployed
to
github-pages
GitHub Actions
Inactive
jonrohan
temporarily deployed
to
github-pages
GitHub Actions
Inactive
|
Ok, this is ready again. This time I've fixed the issues we saw when shipping last time https://github.com/github/github/pull/351862 |
jonrohan
requested review from
hussam-i-am and
langermank
and removed request for
tbenning
There was a problem hiding this comment.
Mostly LGTM, just a few comment cleanup suggestions and one question:
I see integration PR is failing, is that just because it needs the new changes on the separate dotcom PR? 👀
non-blocking: I'm wondering if this story is working as supposed to. It behaves the same in prod but are the sizes even doing anything here? 😅
| /* 2. + the non-overlapping part of the third and fourth avatar;thiscalcexplained */ | ||
| min-width: calc( |
There was a problem hiding this comment.
| /* 2. + the non-overlapping part of the third and fourth avatar;thiscalcexplained */ | |
| min-width: calc( | |
| /* 2. + the non-overlapping part of the third and fourth avatar */ | |
| min-width: calc( |
|
|
||
| /* 1. avatar size + the non-overlapping part of the second avatar */ | ||
|
|
||
| /* 2. + the non-overlapping part of the third avatar;thiscalcexplained */ |
There was a problem hiding this comment.
| /* 2. + the non-overlapping part of the third avatar;thiscalcexplained */ | |
| /* 2. + the non-overlapping part of the third avatar */ |
|
|
||
| /* 1. avatar size + the non-overlapping part of the second avatar */ | ||
|
|
||
| /* 2. + the border widths of the first two avatars;thiscalcexplained */ |
There was a problem hiding this comment.
| /* 2. + the border widths of the first two avatars;thiscalcexplained */ | |
| /* 2. + the border widths of the first two avatars */ |
| @@ -6,8 +6,6 @@ | |||
| /* stylelint-disable-next-line primer/typography */ | |||
| line-height: 1; | |||
| border-radius: 50%; | |||
| /* stylelint-disable-next-line primer/box-shadow */ | |||
| box-shadow: 0 0 0 1px var(--avatar-borderColor); | |||
There was a problem hiding this comment.
Is this no longer needed?
| // @ts-ignore - it's not allowing CSS properties here | ||
| style={enabled ? (getResponsiveAvatarSizeStyles() as React.CSSProperties) : undefined} |
There was a problem hiding this comment.
I think you can get rid of the ts-ignore and the as React.CSSProperties since it is being aliased within the function
| // @ts-ignore - it's not allowing CSS properties here | |
| style={enabled ? (getResponsiveAvatarSizeStyles() as React.CSSProperties) : undefined} | |
| style={enabled ? getResponsiveAvatarSizeStyles() : undefined} |
Closes https://github.com/github/primer/issues/4024
Changelog
Changed
Update
AvatarStackcomponent to use CSS modules behind the feature flag primer_react_css_modules_teamRollout strategy
Testing & Reviewing
Merge checklist