refactor(collapse): use ngAnimate

[chrisirhc]

Update: After merging #1675, I realized most of that logic can now be moved into collapse, as AngularJS 1.3 supports style options on animation calls. I've refactored it so that accordion and collapse share the same logic.

This is my attempt at #1274 .

[pkozlowski-opensource]

@chrisirhc this looks really cool! I'm going to start rebasing the BS3 branch today to get the BS3 release out of the doors before the New Year and then we are going to focus on AngularJS 1.2.x.

[chrisirhc]

Sounds good. Looking forward to it. 😄 Do tag any issues / pull requests that may need extra hands so I (and others) know which ones to focus on. I looked through the issues tagged with Bootstrap3 and didn't see any that were still awaiting action.

[pkozlowski-opensource]

@chrisirhc I'm just going over PRs trying to merge the ones that are ready / worth merging. Then going to rebase the BS3 branch and create a milestone for the next release - items in the milestone will need the immediate attention. Will ping you guys on the mailing list ([email protected]) with more details today.

[chrisirhc]

Great stuff thank you!

[chrisirhc]

Rebased and updated. Take note that the tests currently don't test the $animate methods. But I'm not sure how to go about testing them or whether they need to be tested.

[mvhecke]

@chrisirhc I think testing is still necessary in this case, because you're using a service. Maybe this topic is useful?

[ajoslin]

Use the expect(element).toHaveClass() helper for better error messages

[chrisirhc]

Just an update, I'm working on refactoring this PR at the moment so that hopefully collapse will just consist of an in class that can easily be tested.

[ajoslin]

I like it! Perhaps we should even consider adding a console.warn() about collapse directive being deprecated, with a link to how to just use it with ng-class.

Currently, it fails to apply to the latest - once you rebase (again! lol) it and fix conflicts I'll test it out.

[chrisirhc]

@ajoslin , rebased!

[bekos]

@chrisirhc Can you check why this is failing? I think you need to replace the $timeout flushes with the $animate ones.

[chrisirhc]

@bekos I'll look at this in a bit. Meanwhile #1878 is more critical as that's breaking carousel when animations are enabled. Could you help me take a look at that? If no major issues I'm hoping to merge that soon.

[rjokelai]

Any progress for this one?

[rjokelai]

It seems that the $animate:before/after/close events are not fired without the inclusion of angular-animate.js in the test libraries as well as modules ngAnimate and mock.animate in the collapse.spec.js. Consequently, the tests checking the height of the element fail.

Furthermore, after adding the aforementioned modules, the lines having

$timeout.flush();

need to be changed to

$animate.flushNext('addClass'); // or 'removeClass'
$timeout.flush();

Moreover, these need to be called each time the states of the collapsed as well as the ng-hide element change.

Currently, I have a working local copy. The only failing test is 'should be shown on initialization if isCollapsed = false without transition', since the $animate possibly needs to be flushed for the in class to be added. Still, I had to do changes like the following example

it('should expand if isCollapsed = false with animation on subsequent use', function() {
  scope.isCollapsed = false;
  scope.$digest();
  scope.isCollapsed = true;
  scope.$digest();
  scope.isCollapsed = false;
  scope.$digest();
  $timeout.flush();
  expect(element.height()).not.toBe(0);
});

to this:

it('should expand if isCollapsed = true with animation on subsequent uses', function() {
  expand();
  collapse();
  expand();
  collapse();
  expect(element.height()).toBe(0);
});

where the expand and collapse functions behave like this

function expand() {
  scope.isCollapsed = false;
  scope.$digest();
  $animate.flushNext('addClass');
  $timeout.flush();
}

[chrisirhc]

Updated this PR to move accordion animation logic into collapse directive. Planning to merge this soon.

[wesleycho]

This LGTM.