Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed issue with multiple CSS transitions #13286

Closed
wants to merge 1 commit into from
Closed

Conversation

tdolsen
Copy link

@tdolsen tdolsen commented Apr 6, 2014

Fixed an issue with the transitions of the carousel, which failed to apply the next, prev, left and right classes when more than one CSS transition was set. This was due to multiple durations being returned as a comma-separated list (e.g. ".6s, 2s") which slice(0, 1) * 1000 would turn into a NaN.

The fix adds another function, findDuration, to the emulateTransitionEnd flow, which returns the highest duration in the comma-separated list as milliseconds. The function won't interfere with existing uses, as integers are ignored.

Fixed an issue with the transitions of the carousel, which failed when
more than one CSS transition was set, due to multiple durations being
return. (E.g. ".6s, 2s".)
@cvrebert cvrebert added the js label Apr 6, 2014
@cvrebert cvrebert added this to the v3.2.0 milestone Apr 6, 2014
@fat
Copy link
Member

fat commented Apr 16, 2014

@tdolsen thanks for the pr

i think if we want to go this route (which seems reasonable), this should be applied across all plugins that use .emulateTransitionEnd… that way we're more consistent internally.

why $active.css('transition-duration').slice(0, -1) * 1000 (only used here) was ever merged… is beyond me :/

@cvrebert
Copy link
Collaborator

@fat It backtraces to #11416, which unfortunately didn't get a review by you.

@fat
Copy link
Member

fat commented Apr 16, 2014

@cvrebert it's ok, that's my bad… I was pretty absent.

I think the idea is good, just want to see it propagated across the other plugins in a consistent way 👍

@mdo
Copy link
Member

mdo commented Apr 17, 2014

My bad yo—I don't even recall merging that O_O.

@cvrebert
Copy link
Collaborator

Competing pull request: #13602

@Harris-Miller
Copy link

Hey guys, I discovered something important that you guys should know after I closed #13602, it pertains to how this pull request effects the flow of emulateTransitionEnd()

.class-name {
  transition: width 0.7s, height 1.0s;
}

the event transitionend will fire twice, once for height, once for width. They way that you have

$el.one($.support.transition, function() { ... }).emulateTransitionEnd(300)

will not work properly with multiple transitions when those transitions have different durations. the callback will always fire on the shortest one, and 'called' in emulateTransitionEnd will be set on that first transitionEnd as well. In your current implementation of this, this pull-request is mute (sorry @tdolsen)

the way I found around this is:

var longestTransition = someFuncToDetermineThat($el)
$el.on($.support.transition.end, function(e, isEmulate) {
  if (e.originalEvent.propertyname !== longestTransition && !isEmulate) return
  else {
    // code here
    $el.off($.support.transition.end)
  }
}).emulateTransitionEnd(300)

and of course emulateTransitionEnd would have to be adjusted as well, the manual trigger would have to be

.trigger($.support.transition.end, true)

and you'd have to do the same .one to .on w/ .off inside the call back

I hope that helps. If I get the time I'll apply this to #13602 and re-open.

@fat
Copy link
Member

fat commented May 22, 2014

I'm kinda thinking that transition duration should be constants set on the constructor. This way we dont have to rely on trying to parse css rules, and in the rare case that someone wants to much with the transition defaults we set in our lib in the css, they have a clear path to updating the values in the js too

@fat
Copy link
Member

fat commented May 22, 2014

given that, i'm going to close this issue for now in favor of this issue: #13656

@fat fat closed this May 22, 2014
@mdo mdo removed this from the v3.2.0 milestone May 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants