Fix carousel transition duration

[MartijnCuppens]

Fixes #25203.

Transition duration is taken from first carousel item instead of static value of 600. This allow us to change the transition-duration with css.

Demo: https://codepen.io/MartijnCuppens/pen/YYEQer
(Note: auto updating is disabled, rerun codepen to see changes)

[Johann-S]

A unit test is required please

[MartijnCuppens]

@Johann-S, I tried writing a unit test for this but encountered some problems with it. It's not easy to check the transition-duration in js. I came up with something like this which uses the performance.now() API:

 QUnit.test('should change transition duration in js when changed in css', function (assert) {
    assert.expect(1)
    var templateHTML = '<div id="myCarousel" class="carousel" data-interval="false">'
      + '<div id="first" class="carousel-item active" style="transition: transform .2s ease-out;">'
      + '<img alt="">'
      + '</div>'
      + '<div id="second" class="carousel-item" style="transition: transform .2s ease-out;">'
      + '<img alt="">'
      + '</div>'
      + '<div id="third" class="carousel-item" style="transition: transform .2s ease-out;">'
      + '<img alt="">'
      + '</div>'
      + '</div>'
    var $template = $(templateHTML)
    var t0
    var t1

    $template.bootstrapCarousel()

    $template.on('slide.bs.carousel', function () {
      t0 = performance.now()
    }).on('slid.bs.carousel', function () {
      t1 = performance.now()
      // Check timing differences here
    })

    $template.bootstrapCarousel('next')

  })

The difference in timings should be the transition-duration, but of course there is some deviation.

Also transition-durations are disabled in unit tests too speed up the tests (I think). This is caused by this function in utils.js:

function transitionEndTest() {
    if (typeof window !== 'undefined' && window.QUnit) {
      return false
    }

    return {
      end: 'transitionend'
    }
  }

Any ideas on how to work around this?

[XhmikosR]

If you use includes here, does babel take care of it or do we need a polyfill?

[XhmikosR]

Or even endsWith?

[MartijnCuppens]

@XhmikosR, just tested this, includes and endsWith are not polyfilled by babel.

[XhmikosR]

Thanks for testing. Not sure if we need to enable something else in our babel config, but it gets out of this PRs scope anyway.

[Johann-S]

Yep I thought about that after I asked you to do that, but instead of adding a QUnit test, a visual test should be fine (the result would be displayed in a the console devtools)

[MartijnCuppens]

@Johann-S, done.

[Johann-S]

Sorry I forget one thing, can you update Carousel documentation about that option ?

[MartijnCuppens]

@Johann-S No problem, just added some documentation.

[XhmikosR]

Can you break this line?

[MartijnCuppens]

I had that before, but this caused a linting error:
https://travis-ci.org/twbs/bootstrap/jobs/328121951

Maybe store parseFloat(transitionDuration) in a variable to save some space?

[XhmikosR]

I'll have a look later myself then. No need for a new variable.

…

On Jan 15, 2018 09:32, "Martijn Cuppens" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In js/src/carousel.js <#25218 (comment)>: > @@ -223,6 +225,22 @@ const Carousel = (($) => { return config } + _getTransitionDuration() { + // Get transition-duration of first element in the carousel + let transitionDuration = $(this._element).find(Selector.ITEM).css('transition-duration') + + // Return 0 carousel item is not found + if (!transitionDuration) { + return 0 + } + + // If multiple durations are defined, take the first + transitionDuration = transitionDuration.split(',')[0] + + // Multiply by 1000 if transition-duration is defined in seconds + return transitionDuration.indexOf('ms') > -1 ? parseFloat(transitionDuration) : parseFloat(transitionDuration) * MILLISECONDS_MULTIPLIER I had that before, but this caused a linting error: https://travis-ci.org/twbs/bootstrap/jobs/328121951 Maybe store parseFloat(transitionDuration) in a variable to save some space? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#25218 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVVtRsFGNDyCSd_Sae8uYYehfpYutQyks5tKv8agaJpZM4RVVFW> .

[mdo]

@Johann-S Can you re-review and accept changes if it looks good?

We can get this in v4 stable if everything else looks good.

[Johann-S]

Can you remove the use of ES6 template here, we aren't in our code source

[XhmikosR]

If we decide to merge this, make sure you squash all the commits into one.

Personally, I understand what this fixes, but I'm a little hesitant to merge this so close to the stable.

It's your call, guys.

[Johann-S]

@mdo any concerns against merging this PR ?

[MartijnCuppens]

TRANSITION_DURATION is now used multiple times across the js files. Imo it's better to move the functionality to calculate the transition-duration from this PR to utils.js so it can be used by carousel.js, collapse.js, modal.js, tooltip.js, alert.js and tabs.js. This will allow the user to alter the transition duration of all js components with css.

Shall I make a new PR for this?

[Johann-S]

yep it would be better in a new PR

[mdo]

@Johann-S Sorry for delayed response, no concerns on my end.