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

Some potential concerns that need discussion #3

Open
tomchentw opened this Issue Nov 21, 2014 · 1 comment

Comments

Projects
None yet
1 participant
@tomchentw

tomchentw commented Nov 21, 2014

Thanks for creating this plugin, I believe it would be really helpful.
However, after doing some review on your code, I found some potential concerns that might need be addressed. So I opened up this issue for discussion.

Missing var declaration in the for..in loop

Not sure if it's required, but I think it's better to include it?
http://git.io/sScXvg
http://git.io/koCI6w
http://git.io/JV1JWg

Modify rendered children properties

I'm not sure is this valid React usage? At my first glance it looks pretty magic. Please correct me if I'm wrong.
L52 in http://git.io/c0mrAA
L75, L78 in http://git.io/9utIZw
L110-L114 in http://git.io/3WFBvA

Some minor issues

Module support, clean interface (propTypes)...etc.

@tomchentw

This comment has been minimized.

Show comment
Hide comment
@tomchentw

tomchentw Feb 24, 2015

I found that the second issue might be fixed by cloneWithProps ?

tomchentw commented Feb 24, 2015

I found that the second issue might be fixed by cloneWithProps ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment