Skip to content

Conversation

jcfilben
Copy link
Collaborator

@jcfilben jcfilben commented Sep 11, 2023

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 in utils/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. See StyledCheckBox.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 approach
Tested with the following story and compared to how this is currently working in grommet (see codesandbox)

import React from 'react';
import styled from 'styled-components';
import { Box } from 'grommet';

const CustomBox = styled(Box)`
  padding: 10px;
`;

export const SimpleBox = () => (
  <>
    <CustomBox forwardedAs="span" background="pink" border>
      box content
    </CustomBox>
    <Box as="span" border>
      regular box
    </Box>
  </>
);

SimpleBox.storyName = 'Simple';

export default {
  title: 'Layout/Box/Simple',
};

How should this be manually tested?

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • userEvent is used in place of fireEvent.
  • 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?

@jcfilben jcfilben changed the title Styled components opt in Styled components withConfig approach Sep 11, 2023
@jcfilben jcfilben marked this pull request as draft September 12, 2023 17:21
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"
Copy link
Collaborator

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)

Copy link
Collaborator Author

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 jcfilben marked this pull request as ready for review September 13, 2023 17:56
@jcfilben jcfilben requested a review from taysea September 13, 2023 17:56
@taysea
Copy link
Collaborator

taysea commented Sep 13, 2023

@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",
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

@jcfilben
Copy link
Collaborator Author

@jcfilben Can you add links to the issues/docs you found related to use of & / &&?

Yes, added a link to the PR description

@jcfilben jcfilben marked this pull request as draft September 25, 2023 22:58
@jcfilben
Copy link
Collaborator Author

Closing in favor of #6947

@jcfilben jcfilben closed this Oct 21, 2023
@jcfilben jcfilben mentioned this pull request Mar 28, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants