instea / react-native-color-picker

Color picker component for IOS/Android
Apache License 2.0
272 stars 65 forks source link

Significantly improving performance #7

Closed tomekvenits closed 6 years ago

tomekvenits commented 7 years ago

Hi, I changed the way color picker gets updated. Now on every color change "setState()" is called. When we call it very often it is bad for performance because when render function is called whole DOM is re rendered in our case.

I use setNativeProps() function to make updates directly to native elements. Here is some documentation to demonstrate how it works: https://facebook.github.io/react-native/docs/direct-manipulation.html

I noticed 2x better performance on my android phone, so I think this update is very important.

sodik82 commented 7 years ago

Hello,

thank you very much for the contribution. It is an excellent idea.

I am a bit afraid of direct mutation of the state (it is also the error reported via eslint!) - official React documentation says 'never do that' :) However I believe the same can be done with setState plus proper implementation of shouldComponentUpdate (our component is already pure - so shallow equality test is enough) . Could you update your PR? And please check if npm run lint is okey.

Thank you in advance.

madox2 commented 6 years ago

@venits I have recently tested color picker performance. It is true that by passing updates via setNativeProps you get much better performance in dev mode. However running in release mode, I didn't notice any significant difference.

While testing your solution I have also found that setNativeProps caused an error in RN when it was called on TouchableOpacity (RN: 0.39.2)