Skip to content

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

Merged
jonrohan merged 8 commits intomainfrom
css_modules_avatar_stack
Sep 24, 2024
Merged

Update AvatarStack component to use CSS modules behind the feature flag primer_react_css_modules_team#5012
jonrohan merged 8 commits intomainfrom
css_modules_avatar_stack

Conversation

@jonrohan
Copy link
Copy Markdown
Member

@jonrohan jonrohan commented Sep 24, 2024

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

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Sep 24, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: c34264f

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 Sep 24, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Sep 24, 2024 •
edited
Loading

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 98.26 KB (+0.28% 🔺)
packages/react/dist/browser.umd.js 98.57 KB (+0.22% 🔺)

@github-actions github-actions bot temporarily deployed to storybook-preview-5012 September 24, 2024 00:09 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-5012 September 24, 2024 17:01 Inactive
@jonrohan jonrohan marked this pull request as ready for review September 24, 2024 17:02
@jonrohan jonrohan requested a review from a team as a code owner September 24, 2024 17:02
@jonrohan jonrohan requested review from joshblack and keithamus and removed request for keithamus September 24, 2024 17:02
jonrohan
jonrohan commented Sep 24, 2024
View reviewed changes
packages/react/src/internal/utils/toggleStyledComponent.tsx
Comment on lines +30 to +32
if (sxProp !== defaultSxProp) {
return <Box as={BaseComponent} {...rest} sx={sxProp} ref={ref} />
}
Copy link
Copy Markdown
Member Author

@jonrohan jonrohan Sep 24, 2024

Choose a reason for hiding this comment

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

@joshblack wanted to make this one change I noticed here, the wrapper function wasn't processing sx props when the flag was turned on and a prop was passed in.

Copy link
Copy Markdown
Member

@joshblack joshblack Sep 24, 2024

Choose a reason for hiding this comment

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

@jonrohan LGTM!

joshblack
joshblack approved these changes Sep 24, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

LGTM! Just had a couple of questions for the e2e tests

packages/react/src/internal/utils/toggleStyledComponent.tsx
Comment on lines +30 to +32
if (sxProp !== defaultSxProp) {
return <Box as={BaseComponent} {...rest} sx={sxProp} ref={ref} />
}
Copy link
Copy Markdown
Member

@joshblack joshblack Sep 24, 2024

Choose a reason for hiding this comment

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

@jonrohan LGTM!

e2e/components/AvatarStack.test.ts
colorScheme: theme,
},
})
for (const featureFlagOn of [true, false]) {
Copy link
Copy Markdown
Member

@joshblack joshblack Sep 24, 2024

Choose a reason for hiding this comment

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

One question, how do you feel about using this format for defining tests and running them:

https://github.com/primer/react/blob/main/e2e/components/Link.test.ts#L5

Also wanted to ask if you knew how to filter tests by describe block names? I wasn't sure how to do it 🤔 It's why I've been using styled-components in the name so that I could filter by that in the test name when running playwright locally

@jonrohan @joshblack
Update hot-chicken-tickle.md
57feba3 
Co-authored-by: Josh Black <joshblack@github.com>
@github-actions github-actions bot temporarily deployed to storybook-preview-5012 September 24, 2024 19:36 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-5012 September 24, 2024 19:45 Inactive
@jonrohan jonrohan enabled auto-merge September 24, 2024 20:01
@jonrohan jonrohan added this pull request to the merge queue Sep 24, 2024
Merged via the queue into main with commit 954170b Sep 24, 2024
@jonrohan jonrohan deleted the css_modules_avatar_stack branch September 24, 2024 20:21
@primer primer bot mentioned this pull request Sep 24, 2024
Merged
@primer-integration
Copy link
Copy Markdown

primer-integration bot commented Sep 25, 2024

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

This was referenced Sep 26, 2024
Merged
Closed
@joshblack joshblack restored the css_modules_avatar_stack branch September 26, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joshblack joshblack joshblack approved these changes

Assignees

No one assigned

Labels

staff Author is a staff member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jonrohan @joshblack