Refactor carousel to work with ngAnimate

[chrisirhc]

This is a much cleaner solution that makes use of ngAnimate as it was intended.

It requires a few lines of CSS that will be added by default (similar to how AngularJS adds CSS for ng-hide).
I'll add a Grunt task that will provide the CSS in a separate file so that users that have CSP can add the CSS files separately.

Supercedes #1878.

Fixes #1350 #1513 #1185 #2062 #2235.

Things left:

• Move CSS into .css file in carousel directory
• Add Grunt task to compile CSS files into JS that will be loaded when the JS is loaded
• Add Grunt task to concatenate CSS into one file for CSP users
• Add prefixes for transition css

[chrisirhc]

@pkozlowski-opensource The one thing I'm not so sure about is where to place the css files. I'm keeping them in the /src/ folder for now.

[bazsi]

I needed ngAnimate to cooperate with the carousel implementation in angular-bootstrap, so I went ahead and merged this branch into my local tree and it worked flawlessly.

I've even reviewed the code (while trying to understand how carousel works and what was changed), and I liked what I saw, even though my JS knowledge is limited.

Is there an ETA when this code can be merged? It'd be lovely to see a new release so I don't have to keep a patched copy.

Thanks

[pkozlowski-opensource]

@bazsi I'm guess I'm bottleneck here....
@chrisirhc Chris, could we try to have a hangout sometime after ngEurope to review / discuss ngAnimate-related changes? I was trying to review your code but there are many parts that I don't understand (I'm sorry, my understanding of ngAnmate is really limited...) so just talking those changes over would help to land them sooner than later. WDYT?

[chrisirhc]

Sounds good to me. Drop me a message when you're available after ngEurope. Meanwhile I'll try to figure out if AngularJS 1.3's animation updates will affect this and whether it can be any more elegant.

[intellix]

This fails now in 1.3 as $ngAnimate functionality return promises. So just need to change:

$animate.addClass(slide, directionDirClass, function() {
    slide.removeClass(directionDirClass + ' ' + direction);
    animationDone();
});

to

$animate.addClass(slide, directionDirClass).then(function() {
    slide.removeClass(directionDirClass + ' ' + direction);
    animationDone();
});

I did do this locally and would have done PR but seems i'm using a really old version of this locally.

[wgorder]

@chrisirhc how is this coming. I am using your branch rather then bower since this component is so broken in the current release its basically unusable imo

[wgorder]

@intellix Thanks for posting that, I was wondering wth was going on :) I wish this stuff would get merged in.

[JackMorrissey]

This fixed the numerous issues I was having with Carousel on IOS. Thanks!

[karianna]

@chrisirhc - This can't be automerged at present...

[douglasduteil]

@chrisirhc I think you should merge it to put an end to it. It is still up to date ?

[wgorder]

before it is merged @intellix comment is applicable and should be added as well.

[chrisirhc]

I'll rebase it in a bit.

[chrisirhc]

Rebased and now it works well with the custom builds.

Note that there's something up with using Angular 1.3 on the demo site. The Custom Builds doesn't seem to work. However, that's a separate issue.

[wgorder]

@chrisirhc did you happen to see my comment about noting @intellix post regarding 1.3 incompatibility? Its a few posts up, but I'll quote it here for posterity (not sure if its related to what you are seeing on the demo site):

This fails now in 1.3 as $ngAnimate functionality return promises. So just need to change:

$animate.addClass(slide, directionDirClass, function() {
    slide.removeClass(directionDirClass + ' ' + direction);
    animationDone();
});
to

$animate.addClass(slide, directionDirClass).then(function() {
    slide.removeClass(directionDirClass + ' ' + direction);
    animationDone();
});
I did do this locally and would have done PR but seems i'm using a really old version of this locally.

[wesleycho]

The only question I have about this check is that there is the potential for nextSlide.$element.data('$$ngAnimateState').disabled to be true - in that case, this would be broken. Do we want to say that we do not support this use case?

[chrisirhc]

What do you think if I do this on the $animate:before event for any slide element (and attach it in the slide directive)?
Since it should only mark $currentTransition = true when the animation has started, and it'll only close if it starts. There's a possible race condition where an animation hasn't started but has been queued, but may not matter so much I think, as it's only meant to prevent double-clicking causing a weird transition?

[chrisirhc]

Also, I was just looking at the animation code (in https://github.com/angular/angular.js/blob/g3_v1_2/src/ngAnimate/animate.js#L856-L861), I think the close event always even if the animation is disabled for the element.

[wesleycho]

Looks like you're right - ignore my comment.

[chrisirhc]

@wgorder , Thanks for the heads up! I just saw that. I'll fix that for Angular 1.3 since we're going for Angular 1.3 compatibility in the next release.

[chrisirhc]

Fixed compatibility with Angular 1.3

[ammar91]

What I would need to do in ui.bootstrap-tpl.js file for these changes to take effect?

[wesleycho]

This PR LGTM.

[chrisirhc]

Merged via 431b9c7 . Thanks!