search

angular-ui / bootstrap

PLEASE READ THE PROJECT STATUS BELOW. Native AngularJS (Angular) directives for Bootstrap. Smaller footprint (20kB gzipped), no 3rd party JS dependencies (jQuery, bootstrap JS) required. Please read the README.md file before submitting an issue!
http://angular-ui.github.io/bootstrap/
MIT License
14.29k stars 6.73k forks source link

Accordion animation is not working #4657

Closed Skakruk closed 8 years ago

Skakruk commented 8 years ago

Fix 3144633 has broken collapse animation, because finally is triggered before animation ends.

http://plnkr.co/edit/pyqrBk?p=preview

Foxandxss commented 8 years ago

That plunker doesn't have animations in it, if you put them:

http://plnkr.co/edit/iTxqx6?p=preview

It fails. But there is a patch in 1.4.5 that fixes it:

http://plnkr.co/edit/Mv0uGh?p=preview

Is that good for you?

Skakruk commented 8 years ago

I guess, ngAnimate contains correctly working version of $animateCss, because when we link it and use done callback - animation is working properly. Introduced in v1.4 angular contains $animateCss inside, and seems it is not working as it should.

Foxandxss commented 8 years ago

I am confused sorry. Let's start from zero. What is the problem with your first plunker? I don't see any problem with that plunker. It doesn't have animations because ngAnimate is not loaded.

Skakruk commented 8 years ago

OK, I got it. There is no need to use $animate nor $animateCss for ui.bootstrap.collapse directive. In bootstrap animations are css-driven using transitions. We just need to add/remove classes in proper time. In original bootstrap it is done with setTimeout. E.g. ui.bootstrap.collapse show procedure is next:

  1. Initially panel body has class .collapse.
  2. We click on trigger and script calculates height on the body and adds class .collapsing, removes .collapse. It has css transition, so there is no need to animate using js. We just start setTimeout for the same period as transition lasts (350ms).
  3. CSS animation ends and so as timeout. On it's callback we remove class .collapsing (to prevent any other animations related to height change), adding .collapse in class and setting height to auto so prevent dynamic content to be hidden.

So I think it would be better to rewrite collapse directory to use the same approach as it is in original bootstrap, or add ngAnimate as required lib :blush:

Foxandxss commented 8 years ago

If you want animations, you should add ngAnimate to it. I don't think there is a reason not to use it, but that is up to you.

Skakruk commented 8 years ago

If you say, that ngAnimate is required for animations, than there is no need to play with bs classes, just use $animateCss, see ui.bootstrap.collapse directive in http://plnkr.co/edit/AvLTUV?p=preview

wesleycho commented 8 years ago

ngAnimate is optional - it gives users the choices as to whether they want the animation or not. No sense making it more restrictive unnecessarily.