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

Carousel does not work with Angular 1.4.0 #3811

Closed smajl closed 9 years ago

smajl commented 9 years ago

EDIT: I have just noticed, that you only support Angular 1.3.x. You can close this if desired.

Carousel does only one cycle, then stops. Sliding manually does not work. There is no such problem with Angular 1.3.16 - just change version number in Plunker and see for yourself.

Angular: 1.4.0 UI-Bootstrap: 0.13.0 Plunker: http://plnkr.co/edit/IlauLdEJoar76cD14BLc?p=preview

wesleycho commented 9 years ago

If I had to guess, this is due to the breaking changes with ngAnimate in 1.4.

Soon the team is going to have a discussion on what needs to be done to get 0.13.1 out the door, then we will be looking into Bootstrap 3.3 support & likely Angular 1.4

dkegle commented 9 years ago

Is there any timeline for support Angular 1.4?

chrisirhc commented 9 years ago

I'm looking forward to supporting 1.4 . Unfortunately, the current carousel animations are a bunch of hacks (my fault) made to run on $animate before $animateCss was introduced. It currently relies on JS and CSS animations running in parallel. However, this is no longer allowed as of 1.4 .

mchlbrnd commented 9 years ago

A bit of experimental stuff on 13.0 because I need it for a project. Perhaps a nice PR

What do you think? Also, how would you go about supporting @chrisirhc on > 1.3; using angular.version to inject differently?

Cheers!

https://github.com/mchlbrnd/bootstrap/tree/carousel-animatecss.

wesleycho commented 9 years ago

$animateCss does not exist in 1.3, and so we cannot move to changeover to use it until we start work on a release to support 1.4 unless we consume $injector directly and execute code depending on the angular version loaded. We are currently working towards a 0.13.1 release of bugfixes, and will work on 1.4 support after then.

Using $injector may not be a bad idea necessarily - any solution that would currently be accepted in master would have to support angular 1.3 though.

wesleycho commented 9 years ago

In order to support that though, we would need some build tooling adjustment to run the tests for angular 1.3 and 1.4

mchlbrnd commented 9 years ago

Indeed $injector + angular.version would work perfectly for now.

I haven't checked all specs for mocked $animate other than carousel, but the latter does not feature it. On the road now..

mchlbrnd commented 9 years ago

Check https://github.com/mchlbrnd/bootstrap/commit/0490507f4652160961bb1ab58998f1cc4fc5a88e!

Let me know if you're interested in a PR and in which version you want this to land. Currently build against 0.13.0.

Cheers!

wesleycho commented 9 years ago

Any PR should be based off of master and be against master.

That commit looks like a promising start, but it would need to be cleaned up.

Perhaps when the test(s) are made/tweaked, tests could be hidden behind an if else conditional, or we may want build tooling to be tweaked. Do you have any thoughts @chrisirhc about how we should handle supporting a temporary dual support of Angular 1.3 and 1.4 simultaneously until we move to Angular 1.4 fully?

chrisirhc commented 9 years ago

@mchlbrnd 's solution looks good (sniffing the version). That way, the tests don't need to be changed as of yet.

@mchlbrnd could you submit a PR? We can start reviewing and get this into the next release.

gytisgreitai commented 9 years ago

Imho this is also broken with 1.3.13. See this plunkr http://plnkr.co/edit/KixcEb1uDNNgdXEzTDCe?p=preview

wesleycho commented 9 years ago

It is not broken with 1.3.13: http://plnkr.co/edit/MdZsM16V3hTohh8Rhdv7?p=preview

gytisgreitai commented 9 years ago

Ok, so 0.13.0 requires ng-animate, while 0.12.0 does not: http://plnkr.co/edit/6i8M63gKMuAWfUYGtbYa?p=preview Also note that my first plunker is taken from https://angular-ui.github.io/bootstrap/ by clicking "Edit in plunker"

wesleycho commented 9 years ago

0.12.0 (& 0.12.1) forces it as a requirement - 0.13.0 makes it optional.

dairyisscary commented 9 years ago

I'm a bit confused here. As I understand it, the actual issue is that $animate does not emit events on the jqlite element anymore; therefore when currentTransition is supposed to be set to null, nothing happens and it locks. @mchlbrnd 's solution also solves this in addition to using $animateCss.

However, the use of $animate.addClass is still completely valid in 1.4. Are there other issues that using $animateCss solves? Perhaps it can be said that $animateCss produces cleaner code, but since support for 1.3 and 1.4 is desired, why would you put this extra code in if the original works in both versions?

wesleycho commented 9 years ago

In 1.4, there is a breaking change where animation by JS & CSS mixed together is forbidden in ngAnimate - this requires use of $animateCss.

rgolea commented 9 years ago

+1

wesleycho commented 9 years ago

Please do not +1 issues, it only adds noise.

aendra-rininsland commented 9 years ago

I'm not sure this is fixed — am using Angular 1.4.3 and UI-Bootstrap 0.13.2, and having ng-animate enabled still definitely breaks the carousel...

wesleycho commented 9 years ago

It is fixed on master