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

Cannot change onFinish or onCancel callbacks on an animation #44

Closed
laaglu opened this issue Dec 15, 2016 · 2 comments
Closed

Cannot change onFinish or onCancel callbacks on an animation #44

laaglu opened this issue Dec 15, 2016 · 2 comments
Labels

Comments

@laaglu
Copy link
Contributor

laaglu commented Dec 15, 2016

Attempting to change the value of onFinish or onCancel on an animation results in an unexpected behavior: The new callback is not called and the callback set by the previous change of the onFinish property is called. Furthermore, it is not possible to reset onFinish or onCancel (make them null again) once they have been set.

I think there is a lifecycle management problem. The values of onFinish or onCancel are set in playable.attachHandlersToPlayer and taken from this.props. If one updates the props on the Animation, componentWillReceiveProps will be called. This calls in turn indirectly invokes playable.attachHandlersToPlayer. At that stage alas, the new props have not yet been set and playable.attachHandlersToPlayer sets onFinish and onCancel to the current props values, not the new ones. It also checks onFinish and onCancel for nullity which prevents resetting these props. This explains the current incorrect behavior.

I have prototyped a fix, as this problem was blocking for me. However it is a bit ugly, I have uploaded it to a branch in my repo if you want to have a look at it though.

@bringking bringking added the bug label Dec 15, 2016
@bringking
Copy link
Owner

@laaglu thanks I see the issue. If you have a fix already, please PR. It would be great if you could write a test that verifies the correct behavior as well. If you don't have time to PR, I can address this as soon as I can. Thanks for your help, I really appreciate it! 👍

@laaglu laaglu mentioned this issue Dec 15, 2016
@laaglu
Copy link
Contributor Author

laaglu commented Dec 15, 2016

I have made a PR with the fix. It only fixes Animation.js, because it is the only part of the library I currently use but the problem may exist in other parts of the library (AnimationGroup ?).

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

No branches or pull requests

2 participants