genexu / react-native-pie-chart

Simple pie chart module for your React Native app
MIT License
111 stars 48 forks source link

Use style proptype #3

Closed mithunm93 closed 7 years ago

mithunm93 commented 7 years ago

As described here: https://facebook.github.io/react-native/releases/0.21/docs/style.html#pass-styles-around, we should pass styles around using the style proptype. This is because we may want to define the style to be passed in within our StyleSheet.create, which returns a number for the style.

genexu commented 7 years ago

In the official document you provided, style is assign to 'View' component, so it can use 'View.propTypes.style' to handle propType.

However, we use ART to generate graph in this package, and style will be pass to Surface component, ART have no complete spec, so i can't be sure 'View.propTypes.style' is suitable for this case.

In addition, if 'View.propTypes.style' is suitable for our package, we also need modify some other code probably.

https://github.com/genexu/react-native-pie-chart/blob/master/src/Pie.js#53

Thanks you for your contribution :))

mithunm93 commented 7 years ago

So I did some digging to see where that style prop actually ends up:

  1. The style is passed into NativeSurfaceView here: https://github.com/facebook/react-native/blob/master/Libraries/ART/ReactNativeART.js#L155

  2. The valid attributes that can be passed into NativeSurfaceView is defined by SurfaceViewAttributes here: https://github.com/facebook/react-native/blob/master/Libraries/ART/ReactNativeART.js#L105

  3. SurfaceViewAttributes seems to consist of ReactNativeViewAttributes.UIView here: https://github.com/facebook/react-native/blob/master/Libraries/ART/ReactNativeART.js#L68

  4. style is a valid attribute, and it seems to be defined by ReactNativeStyleAttributes here: https://github.com/facebook/react-native/blob/909af08f24e3c65e46593a1f66d5484c34dda53c/Libraries/Components/View/ReactNativeViewAttributes.js#L35

  5. ReactNativeStyleAttributes is populated with View, Text, and Image styles here: https://github.com/facebook/react-native/blob/d8748233ca0f42a63c39de8c3a2d84c5c6307c03/Libraries/Components/View/ReactNativeStyleAttributes.js#L24

  6. And if we take a look at ViewStylePropTypes, which is relevant to this PR, we can see that it is indeed the same props defined in the RN View documentation. You can see the source here: https://github.com/facebook/react-native/blob/efcdef711eba82b2905d237ee9a3d094652c37ac/Libraries/Components/View/ViewStylePropTypes.js

If we want to be extra sure that ViewStylePropTypes is the same thing that View.propTypes.style refers to, we can hunt that down as well.

  1. The propTypes value in View is defined here: https://github.com/facebook/react-native/blob/master/Libraries/Components/View/View.js#L97

  2. style within ViewPropType is defined here: https://github.com/facebook/react-native/blob/909af08f24e3c65e46593a1f66d5484c34dda53c/Libraries/Components/View/ViewPropTypes.js#L343

  3. And finally here we can see that stylePropType is referring to ViewStylePropTypes.

In addition to this I've tested out passing a value returned by StyleSheet.create (which is a number) into the style prop defined in react-native-pie-chart and they all seem to work properly. I want to use this module the proper way, but it's annoying to get propType errors every time I load the screen due to it looking for an object. Also, in case you didn't know, View.propTypes.style also accepts objects, so any existing code passing in objects would not break or cause warnings.

genexu commented 7 years ago

I just follow the clue and that seem be reasonable. I also have some test by pass styles to it, it work properly, so i decided to merge this commit, thanks for your contribution again :))

genexu commented 7 years ago

I just patch and publish new version, so you can use npm update react-native-pie-chart to update package now.

mithunm93 commented 7 years ago

I'll be happy to not have my simulator throw warnings at me :) . Thank you for writing this module!