Check for data-interval on the first slide of carousel #31818

mitchellfyi · 2020-10-02T08:52:03Z

When starting a cycle for a carousel, it only checks for a default interval, and not an interval defined on the slide element via data props. This adds a check in before creating the interval to move to the next slide.

Fixes #31716

Johann-S

Hi @mitchellbryson,

Thanks for your work 👍

A few changes left:

Adding a unit test
Creating a private method to update the interval according to the next/current element

js/src/carousel.js

mitchellfyi · 2020-10-02T09:45:53Z

Let me know if you want the patch for V4 too.. #31820

XhmikosR · 2020-10-02T11:45:48Z

We handle backports ourselves usually by cherry picking. And I'd prefer if we did the same here too. Only the tests need to be adapted, but we'll see after this lands.

mitchellfyi · 2020-10-02T12:12:01Z

We handle backports ourselves usually by cherry picking. And I'd prefer if we did the same here too. Only the tests need to be adapted, but we'll see after this lands.

Ok. FYI this change includes using SelectorEngine, V4 doesn't.

XhmikosR · 2020-10-02T12:17:34Z

Hmm, true. I guess your v4-dev PR is fine then; I can change the commit message when merging that later.

XhmikosR · 2020-10-02T15:09:03Z

@mitchellbryson thanks for the PRs! Pretty solid contributions. :)

One last thing before merging: can you please update (or make a new ones) https://codepen.io/Johann-S/pen/zYqeNNO?editors=1010 to print the duration of the first slider too?

mitchellfyi · 2020-10-02T15:18:41Z

@XhmikosR thanks!

Sure, here you go: https://codepen.io/mitchellbryson/pen/poyXXMd

XhmikosR · 2020-10-02T15:39:01Z

Maybe I'm missing something but I'm getting inconsistent/wrong results. Isn't the second slider's duration supposed to be ~2s? I'm getting ~10s.

https://codepen.io/XhmikosR/pen/yLOdmgE?editors=1111

EDIT: and then after the first round is done, I'm getting ~5s for the first slider, 10s for the second. Either I'm missing something or the issue isn't fixed. :/

mitchellfyi · 2020-10-02T16:41:57Z

@XhmikosR Yep same here, it's not working with transitions. Looking into this now.

… end)

mitchellfyi · 2020-10-02T17:04:09Z

The last commit fixes the issue with transitions by setting _activeElement to nextElement at the end of slide() but not after the transition is complete - I'm not familiar enough to know if this is going to be a problem or not so let me know.

A better approach might be to track nextElement on the instance too, but that would require more of a rewrite and I wanted to keep this PR simple until told otherwise.

EDIT: new codepen for testing: https://codepen.io/mitchellbryson/pen/ZEWgBEN?editors=1011

Re-review

XhmikosR · 2020-10-03T09:12:16Z

I checked again with https://codepen.io/XhmikosR/pen/yLOdmgE?editors=1011 and it seems to work right :)

EDIT: BTW the Codepen is mixing v4 CSS and v5 JS, but it seems it doesn't affect it for testing purposes only.
EDIT2: Update the pen to use v5 CSS too

XhmikosR · 2020-10-03T09:31:58Z

@mitchellbryson I don't suppose you can update the tests so the previous regression is caught? If I didn't try with the Codepen, the patch would have been wrong.

mitchellfyi · 2020-10-03T09:35:20Z

@XhmikosR yep, I think that's a very good idea :)

mitchellfyi · 2020-10-03T12:34:00Z

_slide() no longer deals with updating the _config.interval, it only updates the _activeElement (just like it does with updating the active indicators through _setActiveIndicatorElement()) and now only cycle() updates the _config.interval, based on the current _activeElement.

_updateInterval() now uses an already set _activeElement, or it finds one (first run).

Updates to the tests reflect that next() is no longer dealing with intervals, only cycle() is.

js/src/carousel.js

Johann-S · 2020-10-05T12:51:08Z

@XhmikosR thinks LGTM here 👍

XhmikosR · 2020-10-05T12:51:32Z

I'd still like tests to include the previous failed attempt which wasn't caught :/

mitchellfyi · 2020-10-05T13:10:41Z

@XhmikosR is https://github.com/twbs/bootstrap/pull/31818/files#diff-5eae769087ca4f58ffb64ddef2d4b0e1R877 not sufficient? I added the 2nd cycle() call (which was missing before).

XhmikosR · 2020-10-05T13:12:17Z

@mitchellbryson oh I didn't see that patch, sorry. I think it should be covered now, indeed. @Johann-S please confirm that this covers all cases.

mitchellfyi · 2020-10-05T13:13:42Z

I updated the pen to show the 2nd bugfix working too: https://codepen.io/mitchellbryson/pen/ZEWgBEN?editors=1011

XhmikosR · 2020-10-05T13:22:39Z

I can confirm that tests fail propery on main BTW :)

Chrome Headless 85.0.4183.121 (Windows 10) Carousel cycle should get interval from data attribute on the active item element FAILED
        Error: Expected 1814 to equal 7.
            at <Jasmine>
            at UserContext.<anonymous> (js/tests/unit/js/tests/unit/carousel.spec.js:898:41 <- js/tests/unit/carousel.spec.js:16886:43)
            at <Jasmine>
        Error: Expected 1814 to equal 9385.
            at <Jasmine>
            at UserContext.<anonymous> (js/tests/unit/js/tests/unit/carousel.spec.js:903:41 <- js/tests/unit/carousel.spec.js:16889:43)
            at <Jasmine>
Chrome Headless 85.0.4183.121 (Windows 10) Carousel cycle should get interval from data attribute on the active item element FAILED
        Error: Expected 1814 to equal 7.
            at <Jasmine>
            at UserContext.<anonymous> (js/tests/unit/js/tests/unit/carousel.spec.js:898:41 <- js/tests/unit/carousel.spec.js:16886:43)
            at <Jasmine>
        Error: Expected 1814 to equal 9385.
            at <Jasmine>
            at UserContext.<anonymous> (js/tests/unit/js/tests/unit/carousel.spec.js:903:41 <- js/tests/unit/carousel.spec.js:16889:43)
            at <Jasmine>
........................................
Chrome Headless 85.0.4183.121 (Windows 10) Carousel next should update the active element to the next item before sliding FAILED
        Error: Expected null to equal <div id="secondItem" class="carousel-item carousel-item-next carousel-item-left">...</div>.
            at <Jasmine>
            at UserContext.<anonymous> (js/tests/unit/js/tests/unit/carousel.spec.js:656:39 <- js/tests/unit/carousel.spec.js:16779:41)
            at <Jasmine>
Chrome Headless 85.0.4183.121 (Windows 10) Carousel next should update the active element to the next item before sliding FAILED
        Error: Expected null to equal <div id="secondItem" class="carousel-item carousel-item-next carousel-item-left">...</div>.
            at <Jasmine>
            at UserContext.<anonymous> (js/tests/unit/js/tests/unit/carousel.spec.js:656:39 <- js/tests/unit/carousel.spec.js:16779:41)
            at <Jasmine>
...
Chrome Headless 85.0.4183.121 (Windows 10): Executed 450 of 450 (2 FAILED) (5.366 secs / 4.968 secs)
TOTAL: 2 FAILED, 448 SUCCESS

XhmikosR · 2020-10-19T12:45:11Z

FYI @mitchellbryson I just wait for another review; unfortunately it seems we are a little stuck (lack of JS-involved people). I'd like to get this patch into alpha3 and #31820 assuming you reopen it and adapt it after this one is merged.

check for data-interval on the first slide of carousel (twbs#31716)

4b11b31

mitchellfyi requested a review from a team as a code owner October 2, 2020 08:52

Johann-S requested changes Oct 2, 2020

View reviewed changes

js/src/carousel.js Outdated Show resolved Hide resolved

Johann-S added js v5 labels Oct 2, 2020

mitchellfyi added 3 commits October 2, 2020 10:25

add updateInterval method for elements of a carousel

89f0023

add test for carousel interval being set during cycle

a815d88

undo editor lint

4d1d401

undo editor lint fully

6e4ddf9

XhmikosR added backport-to-v4 v4 labels Oct 2, 2020

Johann-S previously approved these changes Oct 2, 2020

View reviewed changes

XhmikosR removed backport-to-v4 v4 labels Oct 2, 2020

Merge branch 'main' into carousel-data-bug

842d8b3

update activeElement as soon as slide has finished (before transition…

31459b9

… end)

mitchellfyi mentioned this pull request Oct 2, 2020

Check for data-interval on the first slide of carousel - v4 #31820

Merged

XhmikosR requested a review from Johann-S October 3, 2020 09:08

only updateInterval before using it

ef0f426

Johann-S reviewed Oct 5, 2020

View reviewed changes

js/src/carousel.js Outdated Show resolved Hide resolved

return earlier when missing element

c0ed1f0

Merge branch 'main' into carousel-data-bug

7fdd0af

XhmikosR requested a review from Johann-S October 5, 2020 13:20

Merge branch 'main' into carousel-data-bug

f8f9358

XhmikosR changed the title ~~Check for data-interval on the first slide of carousel (#31716)~~ Check for data-interval on the first slide of carousel Oct 7, 2020

Merge branch 'main' into carousel-data-bug

46cad45

Merge branch 'main' into carousel-data-bug

9adb0ea

Johann-S approved these changes Oct 31, 2020

View reviewed changes

Merge branch 'main' into carousel-data-bug

0b835b6

XhmikosR merged commit 3a5f9f5 into twbs:main Nov 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for data-interval on the first slide of carousel #31818

Check for data-interval on the first slide of carousel #31818

mitchellfyi commented Oct 2, 2020

Johann-S left a comment

mitchellfyi commented Oct 2, 2020

XhmikosR commented Oct 2, 2020

mitchellfyi commented Oct 2, 2020

XhmikosR commented Oct 2, 2020

XhmikosR commented Oct 2, 2020

mitchellfyi commented Oct 2, 2020

XhmikosR commented Oct 2, 2020 •

edited

Loading

mitchellfyi commented Oct 2, 2020

mitchellfyi commented Oct 2, 2020 •

edited

Loading

XhmikosR commented Oct 3, 2020 •

edited

Loading

XhmikosR commented Oct 3, 2020

mitchellfyi commented Oct 3, 2020

mitchellfyi commented Oct 3, 2020

Johann-S commented Oct 5, 2020

XhmikosR commented Oct 5, 2020

mitchellfyi commented Oct 5, 2020

XhmikosR commented Oct 5, 2020

mitchellfyi commented Oct 5, 2020 •

edited

Loading

XhmikosR commented Oct 5, 2020

XhmikosR commented Oct 19, 2020

Check for data-interval on the first slide of carousel #31818

Check for data-interval on the first slide of carousel #31818

Conversation

mitchellfyi commented Oct 2, 2020

Johann-S left a comment

Choose a reason for hiding this comment

mitchellfyi commented Oct 2, 2020

XhmikosR commented Oct 2, 2020

mitchellfyi commented Oct 2, 2020

XhmikosR commented Oct 2, 2020

XhmikosR commented Oct 2, 2020

mitchellfyi commented Oct 2, 2020

XhmikosR commented Oct 2, 2020 • edited Loading

mitchellfyi commented Oct 2, 2020

mitchellfyi commented Oct 2, 2020 • edited Loading

XhmikosR commented Oct 3, 2020 • edited Loading

XhmikosR commented Oct 3, 2020

mitchellfyi commented Oct 3, 2020

mitchellfyi commented Oct 3, 2020

Johann-S commented Oct 5, 2020

XhmikosR commented Oct 5, 2020

mitchellfyi commented Oct 5, 2020

XhmikosR commented Oct 5, 2020

mitchellfyi commented Oct 5, 2020 • edited Loading

XhmikosR commented Oct 5, 2020

XhmikosR commented Oct 19, 2020

XhmikosR commented Oct 2, 2020 •

edited

Loading

mitchellfyi commented Oct 2, 2020 •

edited

Loading

XhmikosR commented Oct 3, 2020 •

edited

Loading

mitchellfyi commented Oct 5, 2020 •

edited

Loading