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

Carousel: Remove redundant reference to interval=false from docs & tests #36545

Merged
merged 3 commits into from Jun 14, 2022

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Jun 11, 2022

closes #36526

@GeoSot GeoSot added this to In progress in v5.2.0-stable via automation Jun 11, 2022
@GeoSot GeoSot requested a review from a team as a code owner June 11, 2022 09:07
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

From what I've tested the boolean is just interpreted as a number when the interval is set. So data-bs-interval="false" would be 0 (very quick) and data-bs-interval="true" would be 1 (very quick as well). data-bs-interval="false" isn't used anymore to block the autoplay so I agree with this modification and the fact that the boolean support should be removed.

Maybe consider some extra-modifications in this PR:

  • Remove data-bs-interval="false" in carousel.spec.js
  • Add something in the migration guide to help the users?

@GeoSot
Copy link
Member Author

GeoSot commented Jun 11, 2022

Thank you @julien-deramond for your suggestions. I've removed the references from the test file.

I don't think we need to add any migration notice. Wishing I am correct, during the latest refactoring, I didn't change the functionality and the checks that are being inside.

v5.2.0-stable automation moved this from In progress to Reviewer approved Jun 11, 2022
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -314,7 +314,7 @@ const carousel = new bootstrap.Carousel('#myCarousel')
{{< bs-table >}}
| Name | Type | Default | Description |
| --- | --- | --- | --- |
| `interval` | number | `5000` | The amount of time to delay between automatically cycling an item. If `false`, carousel will not automatically cycle. |
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this read:

| `interval` | number | `5000` | The amount of time to delay between automatically cycling an item. If `0`, carousel will not automatically cycle. |

Copy link
Member

Choose a reason for hiding this comment

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

Apparently with 0 the interval is just very short (no interval) so the animation is very quick. There's a cycle when data-bs-ride is set to true or carousel.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with Julien here. Are you ok with it @XhmikosR ? can I proceed?

@XhmikosR XhmikosR merged commit fc24f87 into main Jun 14, 2022
v5.2.0-stable automation moved this from Reviewer approved to Done Jun 14, 2022
@XhmikosR XhmikosR deleted the gs/docs-carousel-patch branch June 14, 2022 13:17
@XhmikosR XhmikosR changed the title Carousel: Remove redundant reference to interval=false from docs Carousel: Remove redundant reference to interval=false from docs & tests Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.2.0-stable
  
Done
Status: Done
Development

Successfully merging this pull request may close these issues.

In 5.2.0-beta1 > different behavior of carousel data-bs-interval="false"
3 participants