Skip to content
This repository has been archived by the owner on Sep 17, 2021. It is now read-only.

added pie animation #227

Merged
merged 3 commits into from
Feb 19, 2018
Merged

added pie animation #227

merged 3 commits into from
Feb 19, 2018

Conversation

melkayam92
Copy link
Contributor

@melkayam92 melkayam92 commented Feb 5, 2018

Thank you for contributing a pull request.

Please ensure that you have signed the CLA.

  • I have signed the CLA

Copy link
Contributor

@marzolfb marzolfb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies for the delay in reviewing.

Thanks for wanting to contribute. If you would like to continue, could you please sign the CLA as instructed above? Thanks.

I have reviewed it and I was unable to make it work without modifications changing references of this.props.animate to this.props.options.animate (as noted in review comments).

Also, it would probably be helpful to add an animated version of the pie chart in the example app (as a way of showing the option exists). Are you willing to add that?

src/Pie.js Outdated

}

_shouldAnim = () => this.props.animate.enabled && this.props.data.length > 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.props.options.animate instead of this.props.animate

src/Pie.js Outdated
this._animationArray.map(a =>
Animated.timing(a, {
toValue: 1,
duration: this.props.animate.duration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.props.options.animate instead of this.props.animate

@marzolfb
Copy link
Contributor

Also of important note, I've added a notice to the project readme that this project is going to be archived on February 28th. If you still want to get this change in before it gets archived we can do that if you can make the requested changes happen fairly soon.

@melkayam92
Copy link
Contributor Author

Hey,

Thanks for the response.
I will make those changes soon and will sign the CLA.

@marzolfb
Copy link
Contributor

Thanks for signing the CLA and providing the example but I have one last minor request - could you add a link to the new example in the main menu of the example app? Thanks!

@melkayam92
Copy link
Contributor Author

Done.

@marzolfb marzolfb merged commit bbd34d0 into capitalone:master Feb 19, 2018
@melkayam92 melkayam92 deleted the pie-animation branch February 19, 2018 18:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants