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

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

Merged
merged 13 commits into from
Nov 12, 2020

Conversation

mitchellfyi
Copy link
Contributor

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

js/tests/unit/carousel.js Outdated Show resolved Hide resolved
@mitchellfyi mitchellfyi marked this pull request as ready for review October 2, 2020 12:12
@mitchellfyi mitchellfyi requested a review from a team as a code owner October 2, 2020 12:12
@mitchellfyi mitchellfyi marked this pull request as draft October 2, 2020 17:05
@mitchellfyi
Copy link
Contributor Author

Waiting on feedback from V5 PR: #31818

@@ -492,6 +503,9 @@ class Carousel {
$(this._element).trigger(slidEvent)
}

// does not wait for the transition to complete
this._activeElement = nextElement
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this change please?

Copy link
Contributor Author

@mitchellfyi mitchellfyi Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this PR is currently out of date (so I reverted it to draft). See #31818

@XhmikosR
Copy link
Member

XhmikosR commented Nov 1, 2020

@mitchellbryson can you bring this up to par with your main PR and reopen it please?

@XhmikosR XhmikosR reopened this Nov 1, 2020
@XhmikosR
Copy link
Member

XhmikosR commented Nov 3, 2020

@mitchellbryson friendly ping :) This is currently the only thing pending in our 4.6.0 project

@mitchellfyi
Copy link
Contributor Author

@XhmikosR sorry for the delay, I'll get on it later this week.

@mitchellfyi mitchellfyi marked this pull request as ready for review November 4, 2020 11:52
@XhmikosR
Copy link
Member

Thanks @mitchellbryson, I really appreciate your help and sticking with us ❤

I'm going to merge this since I tested manually and it seems to do the job (and the diff LGTM anyway).

@XhmikosR XhmikosR changed the title Check for data-interval on the first slide of carousel - v4 (#31716) Check for data-interval on the first slide of carousel - v4 Nov 12, 2020
@XhmikosR XhmikosR merged commit 896e444 into twbs:v4-dev Nov 12, 2020
@mitchellfyi
Copy link
Contributor Author

no worries @XhmikosR, thanks for your help getting it through!

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.

4 participants