From 4b11b31226ab3cdb9f0f644e9cde808942c2783c Mon Sep 17 00:00:00 2001 From: Mitchell Bryson Date: Fri, 2 Oct 2020 09:49:07 +0100 Subject: [PATCH 1/8] check for data-interval on the first slide of carousel (#31716) --- js/src/carousel.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/js/src/carousel.js b/js/src/carousel.js index bd5f3d20403b..5e6bfb31e319 100644 --- a/js/src/carousel.js +++ b/js/src/carousel.js @@ -181,6 +181,16 @@ class Carousel { } if (this._config && this._config.interval && !this._isPaused) { + this._activeElement = SelectorEngine.findOne(SELECTOR_ACTIVE_ITEM, this._element) + + if (this._activeElement) { + const activeElementInterval = parseInt(this._activeElement.getAttribute('data-interval'), 10) + + if (activeElementInterval) { + this._config.interval = activeElementInterval + } + } + this._interval = setInterval( (document.visibilityState ? this.nextWhenVisible : this.next).bind(this), this._config.interval From 89f00232ebcbc860e9c6311bb01dc40ac843d437 Mon Sep 17 00:00:00 2001 From: Mitchell Bryson Date: Fri, 2 Oct 2020 10:25:36 +0100 Subject: [PATCH 2/8] add updateInterval method for elements of a carousel --- js/src/carousel.js | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/js/src/carousel.js b/js/src/carousel.js index 5e6bfb31e319..f9a6344707b0 100644 --- a/js/src/carousel.js +++ b/js/src/carousel.js @@ -184,11 +184,7 @@ class Carousel { this._activeElement = SelectorEngine.findOne(SELECTOR_ACTIVE_ITEM, this._element) if (this._activeElement) { - const activeElementInterval = parseInt(this._activeElement.getAttribute('data-interval'), 10) - - if (activeElementInterval) { - this._config.interval = activeElementInterval - } + this._updateInterval(this._activeElement) } this._interval = setInterval( @@ -376,7 +372,7 @@ class Carousel { const activeIndex = this._getItemIndex(activeElement) const lastItemIndex = this._items.length - 1 const isGoingToWrap = (isPrevDirection && activeIndex === 0) || - (isNextDirection && activeIndex === lastItemIndex) + (isNextDirection && activeIndex === lastItemIndex) if (isGoingToWrap && !this._config.wrap) { return activeElement @@ -419,6 +415,17 @@ class Carousel { } } + _updateInterval(element) { + const elementInterval = parseInt(element.getAttribute('data-interval'), 10) + + if (elementInterval) { + this._config.defaultInterval = this._config.defaultInterval || this._config.interval + this._config.interval = elementInterval + } else { + this._config.interval = this._config.defaultInterval || this._config.interval + } + } + _slide(direction, element) { const activeElement = SelectorEngine.findOne(SELECTOR_ACTIVE_ITEM, this._element) const activeElementIndex = this._getItemIndex(activeElement) @@ -473,13 +480,7 @@ class Carousel { activeElement.classList.add(directionalClassName) nextElement.classList.add(directionalClassName) - const nextElementInterval = parseInt(nextElement.getAttribute('data-interval'), 10) - if (nextElementInterval) { - this._config.defaultInterval = this._config.defaultInterval || this._config.interval - this._config.interval = nextElementInterval - } else { - this._config.interval = this._config.defaultInterval || this._config.interval - } + this._updateInterval(nextElement) const transitionDuration = getTransitionDurationFromElement(activeElement) From a815d88b488b4d79dffb9ad5a9ed3cf23179f73e Mon Sep 17 00:00:00 2001 From: Mitchell Bryson Date: Fri, 2 Oct 2020 10:26:04 +0100 Subject: [PATCH 3/8] add test for carousel interval being set during cycle --- js/tests/unit/carousel.spec.js | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/js/tests/unit/carousel.spec.js b/js/tests/unit/carousel.spec.js index 61ca52cb2923..fe71e3bc2fc2 100644 --- a/js/tests/unit/carousel.spec.js +++ b/js/tests/unit/carousel.spec.js @@ -876,6 +876,29 @@ describe('Carousel', () => { expect(window.setInterval).toHaveBeenCalled() expect(window.clearInterval).toHaveBeenCalled() }) + + it('should get interval from data attribute in individual item', () => { + fixtureEl.innerHTML = [ + '' + ].join('') + + const carouselEl = fixtureEl.querySelector('#myCarousel') + const carousel = new Carousel(carouselEl, { + interval: 1814 + }) + + expect(carousel._config.interval).toEqual(1814) + + carousel.cycle() + + expect(carousel._config.interval).toEqual(7) + }) }) describe('to', () => { From 4d1d4011f276b0f60ad448b620873cfa416982d0 Mon Sep 17 00:00:00 2001 From: Mitchell Bryson Date: Fri, 2 Oct 2020 10:44:44 +0100 Subject: [PATCH 4/8] undo editor lint --- js/src/carousel.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/carousel.js b/js/src/carousel.js index f9a6344707b0..40f76d11426d 100644 --- a/js/src/carousel.js +++ b/js/src/carousel.js @@ -372,7 +372,7 @@ class Carousel { const activeIndex = this._getItemIndex(activeElement) const lastItemIndex = this._items.length - 1 const isGoingToWrap = (isPrevDirection && activeIndex === 0) || - (isNextDirection && activeIndex === lastItemIndex) + (isNextDirection && activeIndex === lastItemIndex) if (isGoingToWrap && !this._config.wrap) { return activeElement From 6e4ddf97d205c299f3e78a23d5a02308b2673970 Mon Sep 17 00:00:00 2001 From: Mitchell Bryson Date: Fri, 2 Oct 2020 10:47:35 +0100 Subject: [PATCH 5/8] undo editor lint fully --- js/src/carousel.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/carousel.js b/js/src/carousel.js index 40f76d11426d..d6eb7c6be11e 100644 --- a/js/src/carousel.js +++ b/js/src/carousel.js @@ -372,7 +372,7 @@ class Carousel { const activeIndex = this._getItemIndex(activeElement) const lastItemIndex = this._items.length - 1 const isGoingToWrap = (isPrevDirection && activeIndex === 0) || - (isNextDirection && activeIndex === lastItemIndex) + (isNextDirection && activeIndex === lastItemIndex) if (isGoingToWrap && !this._config.wrap) { return activeElement From 31459b92455256dec4c6f5dd539fa918122252ba Mon Sep 17 00:00:00 2001 From: Mitchell Bryson Date: Fri, 2 Oct 2020 17:57:05 +0100 Subject: [PATCH 6/8] update activeElement as soon as slide has finished (before transition end) --- js/src/carousel.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/js/src/carousel.js b/js/src/carousel.js index d6eb7c6be11e..79f6fa29f683 100644 --- a/js/src/carousel.js +++ b/js/src/carousel.js @@ -181,7 +181,7 @@ class Carousel { } if (this._config && this._config.interval && !this._isPaused) { - this._activeElement = SelectorEngine.findOne(SELECTOR_ACTIVE_ITEM, this._element) + this._activeElement = this._activeElement || SelectorEngine.findOne(SELECTOR_ACTIVE_ITEM, this._element) if (this._activeElement) { this._updateInterval(this._activeElement) @@ -516,6 +516,9 @@ class Carousel { }) } + // does not wait for the transition to complete + this._activeElement = nextElement + if (isCycling) { this.cycle() } From ef0f4266e9af7345d1b61621ec0dc7cd5ddfdb0a Mon Sep 17 00:00:00 2001 From: Mitchell Bryson Date: Sat, 3 Oct 2020 13:26:36 +0100 Subject: [PATCH 7/8] only updateInterval before using it --- js/src/carousel.js | 30 +++++++++++++----------------- js/tests/unit/carousel.spec.js | 23 +++++++++++++---------- 2 files changed, 26 insertions(+), 27 deletions(-) diff --git a/js/src/carousel.js b/js/src/carousel.js index 79f6fa29f683..032937b44d2c 100644 --- a/js/src/carousel.js +++ b/js/src/carousel.js @@ -181,11 +181,7 @@ class Carousel { } if (this._config && this._config.interval && !this._isPaused) { - this._activeElement = this._activeElement || SelectorEngine.findOne(SELECTOR_ACTIVE_ITEM, this._element) - - if (this._activeElement) { - this._updateInterval(this._activeElement) - } + this._updateInterval() this._interval = setInterval( (document.visibilityState ? this.nextWhenVisible : this.next).bind(this), @@ -415,14 +411,18 @@ class Carousel { } } - _updateInterval(element) { - const elementInterval = parseInt(element.getAttribute('data-interval'), 10) + _updateInterval() { + const element = this._activeElement || SelectorEngine.findOne(SELECTOR_ACTIVE_ITEM, this._element) - if (elementInterval) { - this._config.defaultInterval = this._config.defaultInterval || this._config.interval - this._config.interval = elementInterval - } else { - this._config.interval = this._config.defaultInterval || this._config.interval + if (element) { + const elementInterval = parseInt(element.getAttribute('data-interval'), 10) + + if (elementInterval) { + this._config.defaultInterval = this._config.defaultInterval || this._config.interval + this._config.interval = elementInterval + } else { + this._config.interval = this._config.defaultInterval || this._config.interval + } } } @@ -471,6 +471,7 @@ class Carousel { } this._setActiveIndicatorElement(nextElement) + this._activeElement = nextElement if (this._element.classList.contains(CLASS_NAME_SLIDE)) { nextElement.classList.add(orderClassName) @@ -480,8 +481,6 @@ class Carousel { activeElement.classList.add(directionalClassName) nextElement.classList.add(directionalClassName) - this._updateInterval(nextElement) - const transitionDuration = getTransitionDurationFromElement(activeElement) EventHandler.one(activeElement, TRANSITION_END, () => { @@ -516,9 +515,6 @@ class Carousel { }) } - // does not wait for the transition to complete - this._activeElement = nextElement - if (isCycling) { this.cycle() } diff --git a/js/tests/unit/carousel.spec.js b/js/tests/unit/carousel.spec.js index fe71e3bc2fc2..a4bd1bac86a5 100644 --- a/js/tests/unit/carousel.spec.js +++ b/js/tests/unit/carousel.spec.js @@ -636,27 +636,24 @@ describe('Carousel', () => { carousel.next() }) - it('should get interval from data attribute in individual item', () => { + it('should update the active element to the next item before sliding', () => { fixtureEl.innerHTML = [ '' ].join('') const carouselEl = fixtureEl.querySelector('#myCarousel') - const carousel = new Carousel(carouselEl, { - interval: 1814 - }) - - expect(carousel._config.interval).toEqual(1814) + const secondItemEl = fixtureEl.querySelector('#secondItem') + const carousel = new Carousel(carouselEl) carousel.next() - expect(carousel._config.interval).toEqual(7) + expect(carousel._activeElement).toEqual(secondItemEl) }) it('should update indicators if present', done => { @@ -877,18 +874,19 @@ describe('Carousel', () => { expect(window.clearInterval).toHaveBeenCalled() }) - it('should get interval from data attribute in individual item', () => { + it('should get interval from data attribute on the active item element', () => { fixtureEl.innerHTML = [ '' ].join('') const carouselEl = fixtureEl.querySelector('#myCarousel') + const secondItemEl = fixtureEl.querySelector('#secondItem') const carousel = new Carousel(carouselEl, { interval: 1814 }) @@ -898,6 +896,11 @@ describe('Carousel', () => { carousel.cycle() expect(carousel._config.interval).toEqual(7) + + carousel._activeElement = secondItemEl + carousel.cycle() + + expect(carousel._config.interval).toEqual(9385) }) }) From c0ed1f041e272627927630b2aa58e8785c16305b Mon Sep 17 00:00:00 2001 From: Mitchell Bryson Date: Mon, 5 Oct 2020 14:11:59 +0100 Subject: [PATCH 8/8] return earlier when missing element --- js/src/carousel.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/js/src/carousel.js b/js/src/carousel.js index 032937b44d2c..ed8e2b89a67c 100644 --- a/js/src/carousel.js +++ b/js/src/carousel.js @@ -414,15 +414,17 @@ class Carousel { _updateInterval() { const element = this._activeElement || SelectorEngine.findOne(SELECTOR_ACTIVE_ITEM, this._element) - if (element) { - const elementInterval = parseInt(element.getAttribute('data-interval'), 10) + if (!element) { + return + } - if (elementInterval) { - this._config.defaultInterval = this._config.defaultInterval || this._config.interval - this._config.interval = elementInterval - } else { - this._config.interval = this._config.defaultInterval || this._config.interval - } + const elementInterval = parseInt(element.getAttribute('data-interval'), 10) + + if (elementInterval) { + this._config.defaultInterval = this._config.defaultInterval || this._config.interval + this._config.interval = elementInterval + } else { + this._config.interval = this._config.defaultInterval || this._config.interval } }