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

withTheme hoist static (fix #596) #597

Closed
wants to merge 2 commits into from
Closed

Conversation

brunolemos
Copy link
Member

@brunolemos brunolemos commented Mar 18, 2017

Fix #596.

For reference: react-navigation/react-navigation#381

--
The big yarn changes happens on master too, I think yarn.lock is outdated.

@mxstbr-bot
Copy link

mxstbr-bot commented Mar 18, 2017

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

Generated by 🚫 dangerJS

@brunolemos brunolemos requested review from mxstbr and k15a and removed request for mxstbr March 21, 2017 16:14
@mxstbr
Copy link
Member

mxstbr commented Apr 1, 2017

Sorry, which issues is this fixing? I don't quite understand the problem here.

@brunolemos
Copy link
Member Author

@mxstbr

If you use react-navigation, for example, use can configure a scene using a static property:

class MyScene extends React.Component
    static navigationOptions {
        title: 'My page',
    }
}

But, if I use withTheme in this component, the static property will be "lost" and react-navigation won't have access to it, because it will be only in the underlying component, not in the new component created by withTheme.

This PR makes the static properties be passed to the new component using the same approach redux uses on their connect HoC.

@k15a
Copy link
Member

k15a commented Apr 6, 2017

Is there a disussion on redux why they implemented it? I don't understand why react-navigation isn't using props instead.

@kitten
Copy link
Member

kitten commented Apr 19, 2017

Hiya, I think it makes more sense to add this to v2, to prevent further divergence of the two branches, and to prepare for this major release.

I've opened a new PR over at #712 that does the same thing. Also cleaned it up a little and added a displayName for fun and giggles.

@kitten kitten closed this Apr 19, 2017
@kitten kitten deleted the withtheme-hoist-static branch April 19, 2017 23:30
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

5 participants