-
Notifications
You must be signed in to change notification settings - Fork 1k
Styled components withConfig approach #6941
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
Styled components withConfig approach #6941
Conversation
package.json
Outdated
@@ -51,15 +51,15 @@ | |||
"peerDependencies": { | |||
"react": "^16.6.1 || ^17.0.0 || ^18.0.0", | |||
"react-dom": "^16.6.1 || ^17.0.0 || ^18.0.0", | |||
"styled-components": "^6.0.0" | |||
"styled-components": "^5.1.0 || ^6.0.0", | |||
"stylis": "^4.0.0" |
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.
styled-components 6 lists stylis ^4.3.0 as a dependencies. Does this need to be a peer dependency? (especially since it isn't a dependency for styled-components 5)
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.
Good note, I'll remove stylis from the peer dependencies
@jcfilben Can you add links to the issues/docs you found related to use of |
package.json
Outdated
@@ -136,7 +137,8 @@ | |||
"require-reload": "^0.2.2", | |||
"simple-git": "^3.19.1", | |||
"storybook": "7.0.20", | |||
"styled-components": "5.3.9", | |||
"styled-components": "^6.0.0", | |||
"stylis": "^4.0.0", |
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.
Will stylis
be included when we install styled-components 6
?
I think we can remove.
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 was thinking stylis was a PeerDependency of Styled Components but looks like it is just a regular dependency so yes I think we can remove it here.
Yes, added a link to the PR description |
Closing in favor of #6947 |
What does this PR do?
Upgrade to use styled components v6
Styled components that are styling native html components are now using
withConfig(styledComponentsConfig)
.styledComponentsConfig
is defined inutils/styles.js
. The config filters out any props that are not recognized as native html props. For styled components that are styling Grommet components we are not filtering out anything and letting all props pass through. Each Grommet component eventually gets rendered as a native html element and this is where we want the prop filtering to occur, not before then.Stylis selector patterns require a prepended
&
or&&
. I made this adjustment in some files. SeeStyledCheckBox.js
as an example. Styled components documentation on this:https://styled-components.com/docs/basics#pseudoelements-pseudoselectors-and-nesting
Where should the reviewer start?
What testing has been done on this PR?
Tested that
@container
works with this approachTested with the following story and compared to how this is currently working in grommet (see codesandbox)
How should this be manually tested?
Do Jest tests follow these best practices?
screen
is used for querying.userEvent
is used in place offireEvent
.asFragment()
is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
Screenshots (if appropriate)
Do the grommet docs need to be updated?
Should this PR be mentioned in the release notes?
Is this change backwards compatible or is it a breaking change?