-
Notifications
You must be signed in to change notification settings - Fork 598
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.
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
Docs for process for refactoring a Primer React Component to CSS modules #5135
Conversation
|
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! Left a couple of comments 👀
2. **Feature Flagging:** | ||
- Add a feature flag to toggle the `sx` prop for controlled rollout (staff shipping). | ||
3. **Create CSS Module:** | ||
- Add a corresponding `{Component}.module.css` file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be a good place to talk about how we use :where()
for managing specificity or would this be a different doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could just link to the doc I'm writing here when its done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would fall into the css authoring doc https://github.com/primer/react/pull/5037/files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start! Just left some ideas
|
||
1. **Check for `className` and `style` Prop:** | ||
- Ensure the component accepts a `className` for styling from outside of primer/react. | ||
- Ensure the component accepts a `style` prop for more dynamic styling like positioning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to explicitly set this as a prop, or does it just come for free? CC @joshblack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@langermank it will come for free with {...props}
most of the time but it may be good for us to make both explicit to avoid issues like:
<MyComponent
className="hi"
styles={{ /* ... */ }}
{...props}
/>
Where either could be overridden instead of merged like we would want
2. **Feature Flagging:** | ||
- Add a feature flag to toggle the `sx` prop for controlled rollout (staff shipping). | ||
3. **Create CSS Module:** | ||
- Add a corresponding `{Component}.module.css` file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could just link to the doc I'm writing here when its done
Co-authored-by: Katie Langerman <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic!
Closes https://github.com/github/primer/issues/3657
This PR documents how to migrate a styled component into a CSS module in primer react.