Forward shouldForwardProp through withComponent #1668
Conversation
🦋 Changeset is good to goLatest commit: 81a6877 We got this. This PR includes changesets to release 6 packages
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 |
Codecov Report
|
|
Could you branch your change out from the |
Sure, reconciling things now. |
|
Not sure exactly why, but the diff includes stuff that it shouldn't. Maybe it would be best to just close this PR and reopen it after branching away from |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 81a6877:
|
I thought I pushed, sorry -- its been reconciled now. |
packages/styled/src/base.js
Outdated
| ? propName => | ||
| nextOptions.shouldForwardProp(propName) && | ||
| defaultShouldForwardProp(propName) | ||
| : defaultShouldForwardProp |
There was a problem hiding this comment.
My understanding here is that we want to pipe the shouldForwardProp calls like a funnel (C -> B -> A) using either the supplied shouldForwardProp falling back to the composed defaultShouldForwardProp of the parent.
packages/styled/__tests__/styled.js
Outdated
| expect(shouldForwardPropA).toHaveBeenNthCalledWith(1, 'as') | ||
| expect(shouldForwardPropA).toHaveBeenNthCalledWith(2, 'disabled') | ||
|
|
||
| expect(tree).toMatchSnapshot() |
There was a problem hiding this comment.
I figured it would be good to be a bit more explicit with this test and the calls made to shouldForwardProp on each withComponent
- Fixes a bug that prevented `shouldForwardProp` from forwarding to components created via `withComponent`; Fixes emotion-js#1394 - Compacts `createStyled`'s options argument
packages/styled/src/base.js
Outdated
| nextTag = { ...nextTag } | ||
| nextTag.__emotion_real = nextTag | ||
| nextTag.__emotion_styles = nextTag.__emotion_styles.slice(1) | ||
| } |
There was a problem hiding this comment.
If we don't remove the prepended label for nextTag we end up with duplicate labels.
- Fixes duplicate labels when composing components. - Allows for correctly setting the label for the new composed component.
|
@mitchellhamilton could you take this over? I'm afraid I won't have time to finish reviewing this right now. |
Absolutely, no problem! |
| })() | ||
|
|
||
| const ComposedDiv = styled(StyledDiv, { | ||
| shouldForwardProp: () => true | ||
| shouldForwardProp: prop => prop !== 'bar' |
There was a problem hiding this comment.
Why these changes? Was this test failing with the changes to styled here? If the original test here was failing, I think that's a problem.
There was a problem hiding this comment.
I don't think the previous version of this test was foo - it was supposed to test shouldForwardProp composition but () => true doesn't make any difference in regards to the final behavior.
This test would pass with it or without this shouldForwardProp as the composition uses && so if we fold this shouldForwardProp1(prop) && shouldForwardProp2(prop) to shouldForwardProp1(prop) && true then all we really have is shouldForwardProp1(prop)
…mponent` (emotion-js#1668) * Forward shouldForwardProp through withComponent - Fixes a bug that prevented `shouldForwardProp` from forwarding to components created via `withComponent`; Fixes emotion-js#1394 - Compacts `createStyled`'s options argument * Fixes an issue with label duplication - Fixes duplicate labels when composing components. - Allows for correctly setting the label for the new composed component. * Refactor how withComponent handles shouldForwardProp * Fix flow errors Co-authored-by: Seth Benjamin <animecyc@Seths-MacBook-Pro.local> Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
What:
shouldForwardPropfrom forwarding to components created viawithComponent; Fixes Ability to set default shouldForwardProp or expose to extended components #1394createStyled's options argumentWhy:
asprop. This sort of begs the question of whether or not thewithComponentfunctionality is needed given we have theasprop; Regardless, they should operate the same if the API exists.How:
shouldForwardProptogether withoptionsandnextOptions.Checklist: