Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(AvatarStack): Convert AvatarStack to CSS modules behind team feature flag #5299

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open

feat(AvatarStack): Convert AvatarStack to CSS modules behind team feature flag #5299

wants to merge 37 commits into from

Conversation

jonrohan
Copy link
Member

@jonrohan jonrohan commented Nov 14, 2024 •
edited
Loading

Closes https://github.com/github/primer/issues/4024

Changelog

Changed

Update AvatarStack component to use CSS modules behind the feature flag primer_react_css_modules_team

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

jonrohan and others added 24 commits September 23, 2024 23:59
@jonrohan
Convert AvatarStack to CSS modules behind primer_react_css_modules_te…
0e8b547 
…am feature flag
@jonrohan
Updating the tests to run with the feature flag on
6f6fbd1
@jonrohan
Create hot-chicken-tickle.md
4f1c263
@jonrohan
Fix tests
53f29df
@jonrohan @joshblack
Update hot-chicken-tickle.md
57feba3 
Co-authored-by: Josh Black <[email protected]>
@jonrohan
Quiet warnings
8858370
@jonrohan
Merge branch 'main' into css_modules_avatar_stack
056bdc7
@jonrohan
Update CSS disables
c34264f
@jonrohan
Merge branch 'main' into css_modules_avatar_stack
260dbba
@jonrohan
Remove feature flag change
70f83e3
@jonrohan
Revert package changes
cdd6c79
@jonrohan
Revert module changes
6d02573
@jonrohan
Merge branch 'main' into css_modules_avatar_stack
9e2ce13
@jonrohan
Add span element to AvatarStackWrapper
dbf6800
@jonrohan
Update snapshot
ba9b70a
@jonrohan
Merge branch 'main' into css_modules_avatar_stack
928cb31
@jonrohan
Merge branch 'main' into css_modules_avatar_stack
6e92067
@jonrohan
Merge branch 'main' into css_modules_avatar_stack
af045d9
@jonrohan
Merge branch 'main' into css_modules_avatar_stack
620365d
@jonrohan
Fix weird merge problem
b564ae7
@jonrohan
important on the box shadow until avatar removes sx
3c77e04
@jonrohan
Update snapshot
62725ce
@jonrohan
Reduce diff
48b4ca3
@jonrohan
Move AvatarStackBody out of AvatarStack
dcb115d
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 14, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 7235818

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 github-actions bot added the staff Author is a staff member label Nov 14, 2024
@github-actions GitHub Actions
Copy link
Contributor

👋 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 github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Nov 14, 2024
@github-actions github-actions bot temporarily deployed to storybook-preview-5299 November 19, 2024 22:08 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-5299 November 19, 2024 22:13 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-5299 November 19, 2024 22:33 Inactive
@jonrohan jonrohan marked this pull request as ready for review November 20, 2024 00:29
@jonrohan jonrohan requested review from a team as code owners November 20, 2024 00:29
@jonrohan
Copy link
Member Author

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 jonrohan requested review from hussam-i-am and langermank and removed request for tbenning November 20, 2024 00:30
francinelucca
francinelucca reviewed Nov 20, 2024
View reviewed changes
Copy link
Member

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 😅

packages/react/src/AvatarStack/AvatarStack.module.css Outdated Show resolved Hide resolved
packages/react/src/AvatarStack/AvatarStack.module.css Outdated Show resolved Hide resolved
packages/react/src/AvatarStack/AvatarStack.module.css Outdated Show resolved Hide resolved
hussam-i-am
hussam-i-am approved these changes Nov 20, 2024
View reviewed changes
packages/react/src/experimental/Skeleton/SkeletonAvatar.module.css
@@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this no longer needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forget the explanation, The box-shadow "border" wasn't showing up in snapshots before, but when avatar stack was converted it started to fail.

packages/react/src/AvatarStack/AvatarStack.tsx Outdated Show resolved Hide resolved
@jonrohan @francinelucca @hussam-i-am
Apply suggestions from code review
276bd5f 
Co-authored-by: Marie Lucca <[email protected]>
Co-authored-by: Hussam Ghazzi <[email protected]>
@github-actions github-actions bot temporarily deployed to storybook-preview-5299 November 25, 2024 15:55 Inactive
@primer-integration
Copy link

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/351802

@primer-integration
Copy link

🔴 golden-jobs completed with status failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@francinelucca francinelucca francinelucca left review comments

@hussam-i-am hussam-i-am hussam-i-am approved these changes

@langermank langermank Awaiting requested review from langermank

Assignees
No one assigned
Labels
integration-tests: failing Changes in this PR cause breaking changes in gh/gh staff Author is a staff member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jonrohan @hussam-i-am @francinelucca