search

capitalone / react-native-pathjs-charts

Android and iOS charts based on react-native-svg and paths-js
Apache License 2.0
879 stars 260 forks source link

Feature/improve smoothline chart #183

Closed AmauryLiet closed 7 years ago

AmauryLiet commented 7 years ago

Thank you for contributing a pull request.

Please ensure that you have signed the CLA.

Submitted on behalf of third-party: kraynel.

marzolfb commented 7 years ago

This looks like a great contribution and I'm reviewing it in more detail now. One thing that I think would be a fantastic addition to this pull request would be showing an example of these new options by either adding a new chart or modifying an existing one in the example app.

marzolfb commented 7 years ago

I think this is a wonderful addition and I'm happy to approve and merge it as-is but I'm going to give you a little time to see if you want to add any modifications to the example app to demonstrate these new option additions. I like that you allow for function inputs in the options you've added. Thanks!

AmauryLiet commented 7 years ago

Thanks for the comments! Adding an example app in the repo seems like a great idea. I'd be happy to do, I may take a few days to add it since I'm lacking time right now, is it ok for you to wait before merging?

marzolfb commented 7 years ago

Yes, I can certainly wait. There's already an example app in the example directory - a new chart can be added easily.

kraynel commented 7 years ago

Thank you @AmauryLiet for this PR, I did not take the time to do it myself at the time. I also signed the CLA.

marzolfb commented 7 years ago

@AmauryLiet Just checking back in - have you had a chance to add an example?

AmauryLiet commented 7 years ago

@marzolfb Thanks for the reminder, I'll be pushing the changes monday

AmauryLiet commented 7 years ago

Hi @marzolfb, finally added an example to the PR where I expose some possible use of the modifications

marzolfb commented 7 years ago

My sincere apologies for the lengthy delay in reviewing this.

Fantastic work. The example is great. Thank you very much.

I'll merge this in for now but I may wait to release and update to npm so I can pull in the latest versions of dependencies to stay current.