CLAassistant
commented
6 years ago
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
Jordan Ware seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.
zewa666
jordan-ware
This fixes the race condition reported in #54 . There was a unit test for this specific scenario, however it was reporting a false positive because the CSS was using an incorrect name - in the actual execution of the test the animation wasn't used at all. Fixing the unit test reported an error on the original code. I then applied my fixes and the unit test began to pass. I've also tested the changes in my own project with various animation durations on add/remove.
Based on a discussion I saw with JODS and another maintainer (I think in templating github to do with ELSE implementation) I see that we want to prematurely end an animation instead of chaining them, so my changes include this which probably helps the race conditions as well. Developers can use the promises returned by the add/remove class to ensure the animations are chained instead if they want.
Here's a basic timeline of the add/remove overlap scenario before the fix:
addClass()animation start:class-addadded50ms delay
removeClass() animation start:
class-removeaddedcurrent state:
class-remove+class-addapplied to element.addClass()animation end callback:classaddedclass-addremovedremoveClass()animation end callback:class-removeremovedcurrent state:
classis applied because of theaddClass()end animation callback, but shouldn't be.