Feature/improve smoothline chart #183
Conversation
|
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. |
|
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! |
|
Thanks for the comments! |
|
Yes, I can certainly wait. There's already an example app in the example directory - a new chart can be added easily. |
| let strokeWidth = typeof this.props.options.strokeWidth !== 'undefined' | ||
| ? this.props.options.strokeWidth | ||
| : '1'; | ||
| let strokeDasharray = typeof this.props.options.strokeDasharray !== 'undefined' |
There was a problem hiding this comment.
Maybe rename the prop as strokeDashArray?
|
Thank you @AmauryLiet for this PR, I did not take the time to do it myself at the time. I also signed the CLA. |
Allow functions as options to specify per graph settings.
AmauryLiet
force-pushed
the
feature/improve-smoothline-chart
branch
from
2bf8b66 to
c8220d9
Compare
Feature/improve smoothline chart
|
@AmauryLiet Just checking back in - have you had a chance to add an example? |
|
@marzolfb Thanks for the reminder, I'll be pushing the changes monday |
|
Hi @marzolfb, finally added an example to the PR where I expose some possible use of the modifications |
|
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. |
Thank you for contributing a pull request.
Please ensure that you have signed the CLA.
Submitted on behalf of third-party: kraynel.