Skip to content

Conversation

@jonrohan
Copy link
Member

@jonrohan jonrohan commented Aug 26, 2024

Part of https://github.com/github/primer/issues/3877

Changelog

Changed

Converting Avatar component to use CSS modules behind the primer_react_css_modules_team feature flag

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

Tests updated for in and outside of feature flag.

Merge checklist

@changeset-bot
Copy link

changeset-bot bot commented Aug 26, 2024

🦋 Changeset detected

Latest commit: 46da581

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 Aug 26, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Aug 27, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 97.24 KB (+0.18% 🔺)
packages/react/dist/browser.umd.js 97.62 KB (+0.16% 🔺)

@github-actions github-actions bot temporarily deployed to storybook-preview-4885 August 27, 2024 19:23 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-4885 August 27, 2024 21:47 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-4885 September 5, 2024 18:23 Inactive
@jonrohan jonrohan marked this pull request as ready for review September 5, 2024 18:53
@jonrohan jonrohan requested a review from a team as a code owner September 5, 2024 18:53
@jonrohan jonrohan requested a review from joshblack September 5, 2024 18:53
@jonrohan jonrohan enabled auto-merge September 5, 2024 18:54
Comment on lines 68 to 75
for (const [key, value] of Object.entries(size)) {
// @ts-ignore - css property
cssSizeVars[`--avatarSize-${key}`] = `${value}px`
}
} else {
// @ts-ignore - css property
cssSizeVars['--avatarSize-regular'] = `${size}px`
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the biggest weirdness I was doing, trying to set CSS vars on the object if multiple sizes were passed in

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me 👍 This is very similar to what we do for ResponsiveValue with data attributes (e.g. data-gap-narrow, data-gap-regular) and makes a lot of sense to do CSS Custom Properties for values like this.

Copy link
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 and some feedback for the @ts-ignore if it's helpful 👍

Comment on lines 68 to 75
for (const [key, value] of Object.entries(size)) {
// @ts-ignore - css property
cssSizeVars[`--avatarSize-${key}`] = `${value}px`
}
} else {
// @ts-ignore - css property
cssSizeVars['--avatarSize-regular'] = `${size}px`
}
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me 👍 This is very similar to what we do for ResponsiveValue with data attributes (e.g. data-gap-narrow, data-gap-regular) and makes a lot of sense to do CSS Custom Properties for values like this.

@jonrohan jonrohan added this pull request to the merge queue Sep 11, 2024
@jonrohan jonrohan removed this pull request from the merge queue due to a manual request Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

staff Author is a staff member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants