refactor(Blankslate): add support for css modules to Blankslate #4810
Conversation
🦋 Changeset detectedLatest commit: 2416b6a 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 |
size-limit report 📦
|
…lankslate-to-css-modules-2
…com:primer/react into refactor/update-blankslate-to-css-modules-2
|
FYI @siddharthkp here was one idea for feature flagging in the classes 👀 |
| return ( | ||
| <span | ||
| className={cx('Blankslate-Visual', { | ||
| [classes.Visual]: enabled, |
There was a problem hiding this comment.
Note for feature flags only: This is cool! It's a little bit cheating because the component already had classNames 😅
| </div> | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
|
|
||
| .Heading { | ||
| font-size: var(--text-title-size-medium, 1.25rem); | ||
| font-weight: var(--text-title-weight-medium, 600); |
There was a problem hiding this comment.
We don't need to add fallbacks for the values here, the PostCSS parser inlines them for us.
Context https://github.com/github/primer/issues/3696
Refactor Blankslate to use CSS Modules behind a feature flag
Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing