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

Use ES2015 classes for subcomponents instead of imported render functions #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cjb
Copy link

@cjb cjb commented Nov 6, 2015

Hi! I know I recommended the Render.call() thing initially, but we've since discovered that you can just import a native class that extends Component and render that, which feels much more React-y, and allows the native subclass to override any method of Component (such as storing its own state if it wants to) rather than just Render().

Here's a PR that shows how that works. It's totally untested, though, please test carefully if you decide you'd like to merge! No worries if you decide you prefer the old method. I just thought I'd share.

@benoitvallon
Copy link
Owner

@cjb very very interesting approach.

I am not going to merge it for 2 reasons (at least now).
First even if more "react-y" I think it makes the code harder to get for people who are not so used to react and the goal of the repo was more to demonstrate how to use react to make the 3 apps rather than having a high technical demo of React, and second I think it would be nice to have the android app merged and structured with a more classical way of doing javascript first.

About the tests, they all passed and the react-native build too so I think your version works great.

About merging, we will see in the future because this approach is so interesting and I think may also help to get a simpler structure when the android version will be merged because thinking in terms of components imbrication may be easier to understand.

Thanks for your suggestion

@cjb
Copy link
Author

cjb commented Nov 8, 2015

Cool, no worries.

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

2 participants