Skip to content
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

Fix defaultProps used instead ThemeProvider after initial render #450

Merged
merged 3 commits into from
Feb 4, 2017
Merged

Conversation

k15a
Copy link
Member

@k15a k15a commented Feb 1, 2017

Closes #445

@k15a k15a requested a review from diegohaz February 1, 2017 21:52
@k15a
Copy link
Member Author

k15a commented Feb 1, 2017

@chadwatson Could you check out if this is working for you?

@chadwatson
Copy link

Sure thing. Give me a few minutes.

Copy link
Member

@diegohaz diegohaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, maybe we could add a test to cover #445 ?

@chadwatson
Copy link

Yes, that fixes the problem! It is still double rendering the child styled component though. Is that a problem?

@k15a
Copy link
Member Author

k15a commented Feb 1, 2017

It's double rendering because you're passing a new object every time and ThemeProvider is checking the reference. In your example they are different so ThemeProvider thinks it is a new Theme.

@k15a
Copy link
Member Author

k15a commented Feb 1, 2017

@diegohaz Do you know how chadwatsons example is different than the test you introduced in #345? I am not seeing any difference.

@diegohaz
Copy link
Member

diegohaz commented Feb 1, 2017

@k15a The difference is that he uses setState after the first render, right?

@xcoderzach
Copy link
Contributor

xcoderzach commented Feb 1, 2017

@k15a I think it's because the props on the Text change without remounting, thus componentWillReceiveProps is called. In this specific case, it's the children prop (the count).

@xcoderzach
Copy link
Contributor

Any time the props on the styled component change without re-mounting this should happen.

@k15a k15a changed the title Fix defaultProps used instead ThemeProvider on first render Fix defaultProps used instead ThemeProvider after initial render Feb 1, 2017
@xcoderzach
Copy link
Contributor

xcoderzach commented Feb 1, 2017

Probably something to fix after this gets landed, but it feels a bit precarious to have this same logic duplicated 4x. 😬

DefaultProps got used after the initial render instead of ThemeProvider.
This test makes sure that this is not happening again.
@mxstbr-bot
Copy link

mxstbr-bot commented Feb 1, 2017

Warnings
⚠️ There are library changes, but not tests. That's OK as long as you're refactoring existing code

Generated by 🚫 dangerJS

@k15a
Copy link
Member Author

k15a commented Feb 1, 2017

@xcoderzach I wasn't thinking of the children prop. Added tests now.

@k15a
Copy link
Member Author

k15a commented Feb 1, 2017

Probably something to fix after this gets landed, but it feels a bit precarious to have this same logic duplicated 4x. 😬

Basically the whole StyledNativeComponent.js is a duplicate of StyledComponent.js. But it feels wrong anyway^^

@diegohaz
Copy link
Member

diegohaz commented Feb 3, 2017

@k15a Can you confirm if that test fails without the changes on this PR?

@k15a
Copy link
Member Author

k15a commented Feb 3, 2017

Yeah it's failing without the changes.

Copy link
Member

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipping this asap, sorry for the delay!

@mxstbr mxstbr merged commit 755e4fc into styled-components:master Feb 4, 2017
santaclauze pushed a commit to santaclauze/styled-components that referenced this pull request Mar 22, 2017
kopax pushed a commit to kopax/styled-components that referenced this pull request Mar 22, 2017
kitten added a commit that referenced this pull request Mar 22, 2017
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.

None yet

6 participants