fix(carousel): Animation Callback in Angular 1.4 #3946
Conversation
|
See the discussion in #3811 for guidance on the preferred implementation. |
| $animate.on('addClass', slide.$element, function closeFn14(aniElem, phase) { | ||
| if ( phase === 'close' ) { | ||
| $scope.$currentTransition = null; | ||
| $animate.off('addClass', aniElem); |
There was a problem hiding this comment.
This should also specify closeFn() as the 3rd argument, as there could be other event listeners attached to addClass, this interfere with those event listeners.
|
Did a review. This looks good. I thought at first that we need to check the major version of angular but I realized that if Angular 2 is used, it would probably be a breaking change anyways. |
|
Ideally, shouldn't we migrate over to $animateCss conditionally through usage of $injector similar to the suggestion that was made in the aforementioned issue? It contains explicit support for adding and removing classes, which I would be concerned about code fragility with ngAnimate support in abusing $animate to get 1.4 support given that it has been stated that $animateCss should be used for anything related to CSS animations (and that CSS animations otherwise would not be supported). If this patch is to make it in, I would want to refactor it asap to change over to use $animateCss in the 1.4+ situation, since it could potentially be dangerous. |
|
You're right, we still need to migrate over to I'm not at all opposed to getting the $animateCss refactor in. It's just that if this is the bare minimum, we shouldn't block merging this since we can always do the $animateCss refactor later and things would still be working for a while. |
| slide.$element.one('$animate:close', function closeFn() { | ||
| $scope.$currentTransition = null; | ||
| }); | ||
| if ( angular.version.minor >= 4 ) { |
There was a problem hiding this comment.
Alternatively, test for existence of $animate.on like if ($animate.on) {.
|
I made these minor changes. :) Also i'm with @chrisirhc. From my testing, the use of $animateCss is actually more or less equivalent to the use of .addClass currently. I would not be surprised if .addClass uses animateCss under the sheets anyway (the docs said as much, but I did not bother reading and angular source code). |
|
My comment about $animateCss is mainly due to the official stance on the ngAnimate api - in the future, it is conceivable that $animate will make a breaking change with regards to how it handles animation involving classes with $animate. That said, I am not against getting the change in without $animateCss support, but it makes changing to use $animateCss for 1.4+ more urgent whenever possible to avoid breakage in the future. |
|
I did a build based off of this PR, and it seems like the animation is a little borked - check out the Plunker here to see the UI glitch. |
|
Hm.. indeed, in the Plunker, it looks like the classes from |
$animate no longer triggers events on the element itself, but rather has its own interface for managing these callbacks.
I wasn't sure what the protocal for supporting angular 1.3 vs 1.4 was, so I just put the version check in.
Grunt passed locally and also worked in my manual testing.
Let me know. :)