Skip to content

refactor(Blankslate): add support for css modules to Blankslate - #4810

Merged
joshblack merged 16 commits into
mainfrom
refactor/update-blankslate-to-css-modules-2
Aug 7, 2024
Merged

refactor(Blankslate): add support for css modules to Blankslate#4810
joshblack merged 16 commits into
mainfrom
refactor/update-blankslate-to-css-modules-2

Conversation

@joshblack

@joshblackjoshblack commented Aug 1, 2024

Copy link
Copy Markdown
Member

Context https://github.com/github/primer/issues/3696

Refactor Blankslate to use CSS Modules behind a feature flag

Changelog

New

Changed

  • Update Blankslate component to use CSS Modules
  • Add feature flag to Blankslate component to toggle between styled-components and CSS Modules
  • Use feature flag for VRT in Blankslate
  • Add fallback VRT tests for Blankslate
  • Update script for generating export sizes to use noop plugin for CSS in entrypoints

Removed

Rollout strategy

  • Minor release

Testing & Reviewing

  • Verify that no regressions have occurred in VRT
  • Verify that snapshots have been added for styled-component styles

@changeset-bot

changeset-botBot commented Aug 1, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 2416b6a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
@primer/reactMinor

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-actionsgithub-actionsBot added the staff Author is a staff member label Aug 1, 2024
@github-actions

github-actionsBot commented Aug 1, 2024

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
packages/react/dist/browser.esm.js95.8 KB (0%)
packages/react/dist/browser.umd.js96.19 KB (0%)

@github-actions
github-actionsBottemporarily deployed to storybook-preview-4810 August 1, 2024 21:24 Inactive
@joshblackjoshblack added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Aug 1, 2024
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4810 August 1, 2024 21:43 Inactive
@github-actionsgithub-actionsBot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Aug 1, 2024
…com:primer/react into refactor/update-blankslate-to-css-modules-2
@joshblackjoshblack added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Aug 1, 2024
@joshblack
joshblack marked this pull request as ready for review August 1, 2024 21:49
@joshblack
joshblack requested review from a team as code ownersAugust 1, 2024 21:49
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4810 August 1, 2024 21:53 Inactive
@github-actionsgithub-actionsBot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Aug 1, 2024
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4810 August 1, 2024 22:00 Inactive
@joshblack

Copy link
Copy Markdown
MemberAuthor

FYI @siddharthkp here was one idea for feature flagging in the classes 👀

return (
<span
className={cx('Blankslate-Visual', {
[classes.Visual]: enabled,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note for feature flags only: This is cool! It's a little bit cheating because the component already had classNames 😅

</div>
</div>
)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note for feature flags only: Idk how to feel about 2 completely different blocks. On one hand, it's very clear that one uses classNames without the Styled(component)Blankslate. On the other hand, if StyledBlankslate accepted sx, we wouldn't be able to do this and would need to merge them in some way.

I think this code looks good and we should ship it + we'll need to migrate a couple other components to see what our practices should be

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Agreed, this one really feels like a one off because of how simple it is. In reality I think we'll have more mix-and-match and toggling inline

@joshblack
joshblack added this pull request to the merge queueAug 7, 2024
Merged via the queue into main with commit c0425ffAug 7, 2024
@joshblack
joshblack deleted the refactor/update-blankslate-to-css-modules-2 branch August 7, 2024 16:10
@primerprimerBot mentioned this pull request Aug 7, 2024

.Heading {
font-size: var(--text-title-size-medium, 1.25rem);
font-weight: var(--text-title-weight-medium, 600);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't need to add fallbacks for the values here, the PostCSS parser inlines them for us.

@primerprimerBot mentioned this pull request Oct 18, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

staffAuthor is a staff member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@joshblack@jonrohan@siddharthkp@langermank