Provide a pause button for Carousels #1146

MewenLeHo · 2022-03-10T16:02:26Z

louismaximepiton

I feel weird using the pause button and unpause on hover. IMHO, I would have done a pause button that prevent hover on carousel play/pause behavior

scss/_variables.scss

site/content/docs/5.1/components/carousel.md

js/src/carousel.js

louismaximepiton · 2022-03-16T16:01:35Z

js/src/carousel.js

+        this._element.nextElementSibling.classList.add('play')
+        this._element.nextElementSibling.setAttribute('title', 'Play Carousel')
+        this._element.nextElementSibling.querySelector('span.visually-hidden').innerHTML = 'Play Carousel'
+        this._isPaused = true


IMO, this is dupe with following code

This line is mandatory in order to prevent side effects between the play/pause button VS the already existing 'pause on hover' option.

Without this line, if you set the carousel to pause using the button and hover it with your cursor then the carousel will stay on pause on mouseenter and will restart to cycle on mouseleave but it will be stuck on the current slide.

However it may be due to the bug I found on Bootstrap carousel but we will have to wait for their fix to check it.

js/src/carousel.js

MewenLeHo · 2022-03-18T14:14:01Z

I feel weird using the pause button and unpause on hover. IMHO, I would have done a pause button that prevent hover on carousel play/pause behavior

I do not think that i's a good idea to block an option for users. In my opinion it's better to allow both and to make sure that it will not break is someone want to use both options.

js/src/carousel.js

louismaximepiton · 2022-03-22T08:48:40Z

js/tests/unit/carousel.spec.js

@@ -479,6 +479,22 @@ describe('Carousel', () => {
        }, 10)
      })
    })
+
+    it('should take care of element either passed as a CSS selector or DOM element (Play/Pause button)', () => {


Not sure to understand this test, shouldn't it return the Carousel itself since you call new Carousel ?

As discussed together, it's a basic test used in all components. I think we need to keep it as it is.

scss/_variables.scss

js/tests/unit/carousel.spec.js

louismaximepiton

Two little points to clarify, but LGTM already.

louismaximepiton · 2022-03-25T13:24:02Z

js/tests/unit/carousel.spec.js

+        const carouselEl = fixtureEl.querySelector('#myCarousel')
+        const carousel = new Carousel(carouselEl)
+
+        spyOn(carousel, 'pause').and.callThrough()


Do we need these .callthrough ?

louismaximepiton · 2022-03-25T13:24:57Z

js/tests/unit/carousel.spec.js

+        const mouseOverEvent = createEvent('mouseover')
+        carouselEl.dispatchEvent(mouseOverEvent)
+
+        setTimeout(() => {


Do we need these setTimeout ?

Yes, because of th promise.

louismaximepiton

LGTM if the behavior is approved !

julien-deramond · 2022-04-06T11:12:35Z

FYI approved by the a11y team.

julien-deramond

Haven't finished the review yet but here are some first comments.

js/src/carousel.js

louismaximepiton

Regarding performance, I think it could be a bit better.

Just looked at it, but the example button seems to be not aligned with the carousel

louismaximepiton · 2022-04-11T10:33:48Z

js/src/carousel.js

+    if (this._playPauseButton !== null && this._playPauseButton.classList.contains('pause')) {
+      this._playPauseButton.classList.remove('pause')
+      this._playPauseButton.classList.add('play')
+      this._playPauseButton.setAttribute('title', this._playPauseButton.getAttribute(SELECTOR_CAROUSEL_PLAY_TEXT))


I think we should have a default value in English I guess

js/src/carousel.js

louismaximepiton · 2022-04-11T10:33:03Z

js/tests/unit/carousel.spec.js

+        '    <div class="carousel-item">item 3</div>',
+        '  </div>',
+        '</div>',
+        '<button type="button" class="btn btn-icon btn-secondary pause" data-bs-control="play-button" data-bs-target="#myCarousel" data-bs-play-text="Play Carousel" data-bs-pause-text="Pause Carousel" title="Pause Carousel">',


Maybe you should try with another text or language here ?

site/content/docs/5.1/components/carousel.md

scss/_variables.scss

js/tests/unit/carousel.spec.js

julien-deramond · 2022-04-19T07:23:02Z

Just looked at it, but the example button seems to be not aligned with the carousel

I agree with @louismaximepiton's comment

julien-deramond · 2022-04-19T11:10:29Z

@MewenLeHo There is an issue with JS tests probably linked to the remaining [data-bs-control="play-button"] in carousel.spec.js removed in 4038734:

const buttonPlayPauselEl = fixtureEl.querySelector('[data-bs-control="play-button"]')
const buttonPlayPauselBySelector = new Carousel('[data-bs-control="play-button"]')

I let you fix the tests and then I'll approve + merge 🚀

julien-deramond · 2022-04-19T13:39:54Z

Bundlewatch needs to be updated as well

julien-deramond

🚀

MewenLeHo force-pushed the main-mewenleho-carousels-pause-button branch from 11da3d8 to 47a3cb9 Compare March 10, 2022 16:04

MewenLeHo requested a review from julien-deramond as a code owner March 10, 2022 16:04

MewenLeHo force-pushed the main-mewenleho-carousels-pause-button branch from 577d087 to 05a2203 Compare March 14, 2022 16:12

MewenLeHo requested a review from louismaximepiton March 15, 2022 16:41

louismaximepiton requested changes Mar 16, 2022

View reviewed changes

MewenLeHo requested a review from louismaximepiton March 21, 2022 09:39

louismaximepiton reviewed Mar 22, 2022

View reviewed changes

MewenLeHo requested a review from louismaximepiton March 22, 2022 16:40

louismaximepiton reviewed Mar 25, 2022

View reviewed changes

MewenLeHo force-pushed the main-mewenleho-carousels-pause-button branch from fe7df32 to 455a5b0 Compare April 4, 2022 13:27

MewenLeHo requested a review from louismaximepiton April 4, 2022 14:28

louismaximepiton approved these changes Apr 5, 2022

View reviewed changes

julien-deramond added v5 accessibility back-from-v4 upcoming a11y review and removed upcoming a11y review labels Apr 6, 2022

julien-deramond force-pushed the main-mewenleho-carousels-pause-button branch from 16001a7 to caa8434 Compare April 6, 2022 11:30

julien-deramond reviewed Apr 6, 2022

View reviewed changes

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

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

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

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

MewenLeHo force-pushed the main-mewenleho-carousels-pause-button branch 2 times, most recently from 16001a7 to 520a54b Compare April 6, 2022 13:06

MewenLeHo requested review from louismaximepiton and julien-deramond April 8, 2022 14:33

louismaximepiton reviewed Apr 11, 2022

View reviewed changes

MewenLeHo force-pushed the main-mewenleho-carousels-pause-button branch from 39043f5 to fc3f65b Compare April 13, 2022 15:27

MewenLeHo requested a review from louismaximepiton April 13, 2022 16:01

julien-deramond force-pushed the main-mewenleho-carousels-pause-button branch 2 times, most recently from e401bfa to 31e3fce Compare April 19, 2022 06:51

julien-deramond reviewed Apr 19, 2022

View reviewed changes

site/content/docs/5.1/components/carousel.md Show resolved Hide resolved

scss/_variables.scss Outdated Show resolved Hide resolved

js/tests/unit/carousel.spec.js Outdated Show resolved Hide resolved

MewenLeHo and others added 5 commits April 19, 2022 15:47

feat #1112 (carousel pause button): basic setup

778cc0a

Some Boosted mod fixes

ce2a6e9

feat #1112 (carousel pause button): fix after review

0ed2ec6

Move Boosted mod

6269688

feat #1112 (carousel pause button): fix tests

2c06277

MewenLeHo force-pushed the main-mewenleho-carousels-pause-button branch from c9354f9 to 2c06277 Compare April 19, 2022 13:48

feat #1112 (carousel pause button): fix bundlewatch

f3bcc67

julien-deramond approved these changes Apr 20, 2022

View reviewed changes

julien-deramond merged commit 7f07c73 into main Apr 20, 2022

julien-deramond deleted the main-mewenleho-carousels-pause-button branch April 20, 2022 05:34

MewenLeHo added breaking-change and removed breaking-change labels May 9, 2022

julien-deramond mentioned this pull request Jul 25, 2022

v5.2.0 Ship List #846

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a pause button for Carousels #1146

Provide a pause button for Carousels #1146

MewenLeHo commented Mar 10, 2022 •

edited

louismaximepiton left a comment

louismaximepiton Mar 16, 2022

MewenLeHo Mar 18, 2022

MewenLeHo commented Mar 18, 2022

louismaximepiton Mar 22, 2022

MewenLeHo Mar 22, 2022

louismaximepiton left a comment

louismaximepiton Mar 25, 2022

MewenLeHo Apr 4, 2022

louismaximepiton Mar 25, 2022

MewenLeHo Apr 4, 2022

louismaximepiton left a comment

julien-deramond commented Apr 6, 2022

julien-deramond left a comment

louismaximepiton left a comment

This comment was marked as off-topic.

louismaximepiton Apr 11, 2022

louismaximepiton Apr 11, 2022

julien-deramond commented Apr 19, 2022

julien-deramond commented Apr 19, 2022 •

edited

julien-deramond commented Apr 19, 2022

julien-deramond left a comment

Provide a pause button for Carousels #1146

Provide a pause button for Carousels #1146

Conversation

MewenLeHo commented Mar 10, 2022 • edited

louismaximepiton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MewenLeHo commented Mar 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louismaximepiton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louismaximepiton left a comment

Choose a reason for hiding this comment

julien-deramond commented Apr 6, 2022

julien-deramond left a comment

Choose a reason for hiding this comment

louismaximepiton left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julien-deramond commented Apr 19, 2022

julien-deramond commented Apr 19, 2022 • edited

julien-deramond commented Apr 19, 2022

julien-deramond left a comment

Choose a reason for hiding this comment

MewenLeHo commented Mar 10, 2022 •

edited

julien-deramond commented Apr 19, 2022 •

edited