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
Remove defaultProps usages for functional components #7224
base: master
Are you sure you want to change the base?
Conversation
DCO Remediation Commit for Max Shepel <max@undeletable.name> I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: cd9a5b7 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 14838be I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 1766eed I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: b5e5d26 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: f005071 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 9f56eca I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 019d28b I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 45f81b2 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: b1ca758 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: be3a510 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: a8ed76e I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: e329a7a I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: ccd1a6c I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 87adfd3 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 164ac90 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 3926bc9 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 1abbf1b I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: bc92022 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 50478dd I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 9e94bce I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 2911f33 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 780ede8 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 74b3e2b I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 58770f0 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 7d584c8 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: b36c664 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: bd1a606 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 86687bf I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 302564b I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 3772235 Signed-off-by: Max Shepel <max@undeletable.name>
@undeletable Thank you for your contribution! We are in the process of looking at this just wanted to let you know! |
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.
Thanks for working on this!
In the changes I saw that you are using the attrs
method of styled-components to pass the theme. In the PR notes you referenced this comment #6741 (comment) as the reason for those changes. I think this comment was misinterpreting the original intent of removing defaultProps
.
There is some confusing naming where we have defaultProps
in the react sense and then we have defaultProps
in Grommet that contains the base theme. I think #6741 (comment), misunderstood the issue and was thinking that we would remove the defaultProps
that has the Grommet theme instead of removing the react defaultProps
.
All this to say I don't think the comment is relevant for what we are trying to do here with removing the react defaultProps and we don't need to use the attrs
method.
We did also test to confirm that the base theme is being passed through correctly to components that are not in a Grommet wrapper in react 18.3.0 without using attrs
.
Actually these two entities are connected: Grommet's Another hint for me was that component snapshots are way too different without both React's |
@jcfilben Way 1. Using inside the component function:
Here, the case of using component outside of theme context is handled by using fallback vaue.No need to pass theme as default prop. I just externalized this logic in the custom hook. Way 2. Using inside arguments of styled-components' template function:
Here, the case of using component outside of theme context is handled by passing theme as default prop. So, if Also, I've tried to remove all Please correct me if I'm wrong. |
I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 6cc1019 I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 429627c I, Max Shepel <max@undeletable.name>, hereby add my Signed-off-by to this commit: 5c80f67 Signed-off-by: Max Shepel <max@undeletable.name>
Thanks for the explanation, and sorry for the confusion on my end. Yesterday my team and I had tested in storybook by removing the Grommet wrapper and
I think the direction you've taken with |
If you update your branch with the latest changes from the master branch, the chromatic check should pass |
@jcfilben Thanks for the suggestion, but this branch is up-to-date with master, and chromatic is still failing. The error is 'Missing project token' - I believe that's because I haven't set up one for the fork. |
Hmm you shouldn't need to set anything up on your end, the chromatic token should be accessible from grommet forks when they run on circleci. I'll look into what might be going wrong here |
I'm going to try and push a no-op commit to your PR to see if I can re-trigger the chromatic step on circleci edit: locally when I pull down this PR I have some tests failing, so I didn't end up pushing anything up yet |
For me, some tests for Pagination are slowly running locally - both in master and my branch. I haven't checked yet what's causing the slowness. |
@jcfilben
|
What does this PR do?
Starting from React 19,
defaultProps
for functional components is deprecated. Starting from React 18.3, the corresponding error is thrown for a functional component withdefaultProps
defined.Changes in this PR:
defaultProps
for functional components with default function parameters values;attrs
method ofstyled-components
to pass theme to styled components which might be outside of theme context (required per remove defaultProps from Grommet #6741 (comment));defaultProps
for functional components;Where should the reviewer start?
No specific place to start.
What testing has been done on this PR?
How should this be manually tested?
Use React 18.3+ in an app. Check that styles are applied correctly and no errors about
defaultProps
are thrown into console.Do Jest tests follow these best practices?
screen
is used for querying.asFragment()
is used for snapshot testing.Any background context you want to provide?
React 19 upgrade guide
What are the relevant issues?
#6741
Screenshots (if appropriate)
N/A.
Do the grommet docs need to be updated?
Just the internal ones (if any).
Should this PR be mentioned in the release notes?
Yes.
Is this change backwards compatible or is it a breaking change?
Changes are backwards compatible.