React-Lightbox icon indicating copy to clipboard operation
React-Lightbox copied to clipboard

Some potential concerns that need discussion

Open tomchentw opened this issue 10 years ago • 1 comments

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 avatar Nov 21 '14 15:11 tomchentw

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

tomchentw avatar Feb 24 '15 01:02 tomchentw