React-Lightbox
React-Lightbox copied to clipboard
Some potential concerns that need discussion
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.
I found that the second issue might be fixed by cloneWithProps ?