Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

Carousel does not work with Angular 1.4.0 #3811

Closed
smajl opened this issue Jun 12, 2015 · 20 comments
Closed

Carousel does not work with Angular 1.4.0 #3811

smajl opened this issue Jun 12, 2015 · 20 comments

Comments

@smajl
Copy link

smajl commented Jun 12, 2015

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
Copy link
Contributor

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
Copy link

dkegle commented Jun 19, 2015

Is there any timeline for support Angular 1.4?

@chrisirhc
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

$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
Copy link
Contributor

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
Copy link

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link

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

@wesleycho
Copy link
Contributor

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

@gytisgreitai
Copy link

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
Copy link
Contributor

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

@dairyisscary
Copy link

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
Copy link
Contributor

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
Copy link

rgolea commented Jul 29, 2015

+1

@wesleycho
Copy link
Contributor

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

@wesleycho wesleycho modified the milestones: 0.13.2 (Performance), Backlog Jul 29, 2015
@aendra-rininsland
Copy link

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
Copy link
Contributor

It is fixed on master

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants