refactor(accordion): use ngAnimate for animations #1675
Conversation
ghost
assigned 
|
LGTM, once the todo list is done it seems good. And it still works well without It seems that we could factor these animations out into their own files/folder though? They have a lot in common. |
|
@chrisirhc would love to get a status update on this. :) |
| .animation('.panel-collapse', function () { | ||
| return { | ||
| beforeAddClass: function (element, className, done) { | ||
| if (className == 'in') { |
There was a problem hiding this comment.
any specific reason why you wouldn't use a strict equality operator? (===)
|
@chrisirhc this is old and a bit outdated, I rebased it but there is no animation, I guess I need to learn more about .animation and css animations :P |
|
Link to repro: http://plnkr.co/edit/0Mxq8jdqpDSxb7x0tURa?p=preview |
chrisirhc
force-pushed
the
feature/accordion-ngAnimate
branch
from
679f655 to
31158fe
Compare
|
Rebased and fixed for Angular 1.3. Just needs review. |
chrisirhc
force-pushed
the
feature/accordion-ngAnimate
branch
2 times, most recently
from
6f5a9f9 to
68f6653
Compare
| beforeAddClass: function (element, className, done) { | ||
| if (className == 'in') { | ||
| element | ||
| .css({height: '0'}) |
There was a problem hiding this comment.
collapsing does that for you and it seems to be working without that line.
There was a problem hiding this comment.
Indeed, good catch.
chrisirhc
force-pushed
the
feature/accordion-ngAnimate
branch
from
68f6653 to
5c95c5c
Compare
|
This fixes #2871 as well. |
chrisirhc
force-pushed
the
feature/accordion-ngAnimate
branch
from
5c95c5c to
09fe63c
Compare
- Deprecate collapse module Consider removing it after transition module is deprecated as well. Fixes angular-ui#2871 Closes angular-ui#1675
chrisirhc
changed the title
chrisirhc
force-pushed
the
feature/accordion-ngAnimate
branch
from
09fe63c to
603a4e7
Compare
- Deprecate collapse module Consider removing it after transition module is deprecated as well. Fixes angular-ui#2871 Fixes angular-ui#3141 Closes angular-ui#1675
- Deprecate collapse module Consider removing it after transition module is deprecated as well. Fixes angular-ui#2871 Fixes angular-ui#3141 Closes angular-ui#1675
chrisirhc
force-pushed
the
feature/accordion-ngAnimate
branch
from
603a4e7 to
f9a9b97
Compare
|
I thought we didn't 'merge' PRs? Shouldn't this have been cherry picked instead? |
|
@karianna This is rebased onto master, there's no Merge commit created, so it's the same as cherry-picking. It's just GitHub shows it as "Merged" if the exact commit SHA is preserved after merging. Also FYI, you can only get to this "Merged" state if the user who created the PR created it off the tip of the master and it was merged while no other change has been committed to master. |
Update: Most likely going to change this approach to something similar to what I did for #1772.Not ready yet until carousel is fixed with ngAnimate as ngAnimate breaks carousel.Consider removing the collapse module after transition module is deprecated as well.
@pkozlowski-opensource , the way I see it, ngAnimate makes animations coupled with the template. I think this makes sense as it's a layer of presentation logic. As such, perhaps having class names only in animations (and templates) might be okay.
TODO:
Update when fix(jqLite): triggerHandler support listeners that unbind from the event angular/angular.js#5984 is updated