Skip to content
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

Consolidate react-native MPChart libraries #1

Open
mikemonteith opened this issue Jan 6, 2016 · 9 comments
Open

Consolidate react-native MPChart libraries #1

mikemonteith opened this issue Jan 6, 2016 · 9 comments

Comments

@mikemonteith
Copy link
Owner

No description provided.

@Taakn
Copy link

Taakn commented Feb 10, 2016

Hi @mikemonteith, I was wondering if you were still working on merging your library with @Jpadilla1. Thanks!

@mikemonteith
Copy link
Owner Author

@Taakn That's the plan. Although getting the Android version as complete as possible is the priority for me at the moment. (pull requests welcome for anything ios based)

@Jpadilla1
Copy link

@Taakn I'm on the same path as @mikemonteith to get the iOS version as complete as possible. Along the way, we could merge both libraries when we are happy enough with the completion.

@Taakn
Copy link

Taakn commented Feb 10, 2016

Ok thanks so much for your work on porting MPChart to React Native!

@Taakn
Copy link

Taakn commented Feb 11, 2016

Ok so thanks to you both, I managed to get your chart libraries working on React 0.19.0 on both iOS and Android.

I have a wrapper that works as follows, so I don't think it really matters to have one repo for iOS, and one repo for Android:

if (Platform.OS === 'ios') {
  var { BarChart } = require('react-native-ios-charts');
}

if (Platform.OS === 'android') {
 var BarChart = require('react-native-mpchart').BarChart;
}

module.exports = {
  BarChart: BarChart
};

It gets a little more complicated when I want to get a chart working on both, for instance:

<BarChart
          style={{flex: 1}}
          ///////////////////////////////////////////////
          // android
          ///////////////////////////////////////////////
          data={{ // android: data / ios: config
            dataSets:[{
              values: [1,2,3,4,5,6,7],
              colors: ['#990000'],
              drawValues: false,
            },{
              values: [4,3,4,2,1,3,1],
              colors: ['#009900'],
              drawValues: false,
            }],
            xValues: ["A","B","C","D","E","F","E"],// android
            highlightEnabled: false,
          }}
          gridBackgroundColor="#33990000" // android (transparent red)
          backgroundColor="white" // is (transparent red, alpha does not work)
          touchEnabled={false} // android only
          // Axis for Android
          leftAxis={{
            //minValue: -12,
            //maxValue: 12,
            drawGridLines: false,
            //inverted: true,
          }}
          rightAxis={{
            enabled: false,
          }}
          xAxis={{
            enabled: false,
          }}
          ///////////////////////////////////////////////
          // ios
          ///////////////////////////////////////////////
          config={{ // android: data / ios: config
            dataSets:[{
              values: [1,2,3,4,5,6,7],
              colors: ['#990000'],
              drawValues: false,
            },{
              values: [4,3,4,2,1,3,1],
              colors: ['#009900'],
              drawValues: false,
            }],
            labels: ["A","B","C","D","E","F","E"],// ios
            highlightEnabled: false,
            xAxis: {enabled: false},
            leftAxis: {drawGridLines: false},
            rightAxis: {enabled: false},
            pinchZoomEnabled: {false},
            doubleTapToZoomEnabled: {false},
            highlightPerTap: {false},
            gridBackgroundColor: '990000', // android (transparent red)
            showLegend: {false},
          }}
        />

It's almost the same basically, with the exception that @Jpadilla1 you wrap everything inside a config attribute, and @mikemonteith you use a data attribute to wrap around your datasets.

So I don't know how you want to proceed in terms of interoperability. For me it's not a big deal given that I can write a wrapper for both, but it would be fantastic to have two components that are as similar as possible when you want to create a chart.

Anyway I think you're both doing a fantastic job and I really want to thank you for everything that you're doing, but also for answering my questions when I was trying to get your libraries up and running.

@Jpadilla1
Copy link

@Taakn you are very welcome and that's awesome that you got both of our libraries working!

The reason why I only use 1 property for everything is that in my experience with iOS and the iOS-Charts library, every time I passed a new property it triggered a render. So let's say we have a BarChart component like this

<BarChart data={...} pinchZoomEnabled={false} gridBackgrounColor='99000'/>

That would trigger 3 renders. So to avoid that I use just 1 property config and it only triggers 1 render.
@mikemonteith does this happen to you in your library?

If their is a better way I'm open ears :)

@Taakn
Copy link

Taakn commented Feb 12, 2016

@Jpadilla1 It doesn't sound like that would be standard React component behavior. Maybe because you're using native components? Were you able to measure a difference in performance?

@mikemonteith
Copy link
Owner Author

@Jpadilla1 is the 3x re-render happening in the React layer or native? I think the native layer should be able to handle props changing so as to only re-render when needed but code will need to be written to handle that.
React should handle changing props in it's own way and make it's own performance optimisations.

@Jpadilla1
Copy link

Hey @mikemonteith I'll re visit this issue and get back to this thread.

Let's decide on how the properties are going to be passed over to iOS/Android. I think that since the libraries have a lot of properties that it's best to have a single configuration object with all the properties. That would make the JSX much cleaner and properties can be shared across multiple charts.

Also, I want to finish supporting all the charts of ios-charts (only missing CombinedChart) and then I would like to focus on Android. Do you need help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants