From b9f30903a5a916904c873bd078240b3df743e093 Mon Sep 17 00:00:00 2001 From: GeoSot Date: Wed, 17 Mar 2021 07:58:43 +0200 Subject: [PATCH] Fix carousel RTL and refactor code, fix rtl swipe issues (#32913) * move common code to reusable functions * add/re-factor tests, directionToOrder func * add _orderToDirection tests Co-authored-by: XhmikosR --- .bundlewatch.config.json | 2 +- js/src/carousel.js | 95 ++++++++++++------------ js/tests/unit/carousel.spec.js | 130 +++++++++++++++++++++++++-------- 3 files changed, 152 insertions(+), 75 deletions(-) diff --git a/.bundlewatch.config.json b/.bundlewatch.config.json index 27b998c6201a..58de657940b2 100644 --- a/.bundlewatch.config.json +++ b/.bundlewatch.config.json @@ -38,7 +38,7 @@ }, { "path": "./dist/js/bootstrap.bundle.min.js", - "maxSize": "22.5 kB" + "maxSize": "22.75 kB" }, { "path": "./dist/js/bootstrap.esm.js", diff --git a/js/src/carousel.js b/js/src/carousel.js index a825aaef4812..b14cbd1a23fd 100644 --- a/js/src/carousel.js +++ b/js/src/carousel.js @@ -10,8 +10,8 @@ import { emulateTransitionEnd, getElementFromSelector, getTransitionDurationFromElement, - isVisible, isRTL, + isVisible, reflow, triggerTransitionEnd, typeCheckConfig @@ -56,8 +56,8 @@ const DefaultType = { touch: 'boolean' } -const DIRECTION_NEXT = 'next' -const DIRECTION_PREV = 'prev' +const ORDER_NEXT = 'next' +const ORDER_PREV = 'prev' const DIRECTION_LEFT = 'left' const DIRECTION_RIGHT = 'right' @@ -137,7 +137,7 @@ class Carousel extends BaseComponent { next() { if (!this._isSliding) { - this._slide(DIRECTION_NEXT) + this._slide(ORDER_NEXT) } } @@ -151,7 +151,7 @@ class Carousel extends BaseComponent { prev() { if (!this._isSliding) { - this._slide(DIRECTION_PREV) + this._slide(ORDER_PREV) } } @@ -208,11 +208,11 @@ class Carousel extends BaseComponent { return } - const direction = index > activeIndex ? - DIRECTION_NEXT : - DIRECTION_PREV + const order = index > activeIndex ? + ORDER_NEXT : + ORDER_PREV - this._slide(direction, this._items[index]) + this._slide(order, this._items[index]) } dispose() { @@ -251,23 +251,11 @@ class Carousel extends BaseComponent { this.touchDeltaX = 0 - // swipe left - if (direction > 0) { - if (isRTL()) { - this.next() - } else { - this.prev() - } + if (!direction) { + return } - // swipe right - if (direction < 0) { - if (isRTL()) { - this.prev() - } else { - this.next() - } - } + this._slide(direction > 0 ? DIRECTION_RIGHT : DIRECTION_LEFT) } _addEventListeners() { @@ -350,18 +338,10 @@ class Carousel extends BaseComponent { if (event.key === ARROW_LEFT_KEY) { event.preventDefault() - if (isRTL()) { - this.next() - } else { - this.prev() - } + this._slide(DIRECTION_LEFT) } else if (event.key === ARROW_RIGHT_KEY) { event.preventDefault() - if (isRTL()) { - this.prev() - } else { - this.next() - } + this._slide(DIRECTION_RIGHT) } } @@ -373,19 +353,18 @@ class Carousel extends BaseComponent { return this._items.indexOf(element) } - _getItemByDirection(direction, activeElement) { - const isNextDirection = direction === DIRECTION_NEXT - const isPrevDirection = direction === DIRECTION_PREV + _getItemByOrder(order, activeElement) { + const isNext = order === ORDER_NEXT + const isPrev = order === ORDER_PREV const activeIndex = this._getItemIndex(activeElement) const lastItemIndex = this._items.length - 1 - const isGoingToWrap = (isPrevDirection && activeIndex === 0) || - (isNextDirection && activeIndex === lastItemIndex) + const isGoingToWrap = (isPrev && activeIndex === 0) || (isNext && activeIndex === lastItemIndex) if (isGoingToWrap && !this._config.wrap) { return activeElement } - const delta = direction === DIRECTION_PREV ? -1 : 1 + const delta = isPrev ? -1 : 1 const itemIndex = (activeIndex + delta) % this._items.length return itemIndex === -1 ? @@ -441,17 +420,19 @@ class Carousel extends BaseComponent { } } - _slide(direction, element) { + _slide(directionOrOrder, element) { + const order = this._directionToOrder(directionOrOrder) const activeElement = SelectorEngine.findOne(SELECTOR_ACTIVE_ITEM, this._element) const activeElementIndex = this._getItemIndex(activeElement) - const nextElement = element || (activeElement && this._getItemByDirection(direction, activeElement)) + const nextElement = element || this._getItemByOrder(order, activeElement) const nextElementIndex = this._getItemIndex(nextElement) const isCycling = Boolean(this._interval) - const directionalClassName = direction === DIRECTION_NEXT ? CLASS_NAME_START : CLASS_NAME_END - const orderClassName = direction === DIRECTION_NEXT ? CLASS_NAME_NEXT : CLASS_NAME_PREV - const eventDirectionName = direction === DIRECTION_NEXT ? DIRECTION_LEFT : DIRECTION_RIGHT + const isNext = order === ORDER_NEXT + const directionalClassName = isNext ? CLASS_NAME_START : CLASS_NAME_END + const orderClassName = isNext ? CLASS_NAME_NEXT : CLASS_NAME_PREV + const eventDirectionName = this._orderToDirection(order) if (nextElement && nextElement.classList.contains(CLASS_NAME_ACTIVE)) { this._isSliding = false @@ -524,6 +505,30 @@ class Carousel extends BaseComponent { } } + _directionToOrder(direction) { + if (![DIRECTION_RIGHT, DIRECTION_LEFT].includes(direction)) { + return direction + } + + if (isRTL()) { + return direction === DIRECTION_RIGHT ? ORDER_PREV : ORDER_NEXT + } + + return direction === DIRECTION_RIGHT ? ORDER_NEXT : ORDER_PREV + } + + _orderToDirection(order) { + if (![ORDER_NEXT, ORDER_PREV].includes(order)) { + return order + } + + if (isRTL()) { + return order === ORDER_NEXT ? DIRECTION_LEFT : DIRECTION_RIGHT + } + + return order === ORDER_NEXT ? DIRECTION_RIGHT : DIRECTION_LEFT + } + // Static static carouselInterface(element, config) { diff --git a/js/tests/unit/carousel.spec.js b/js/tests/unit/carousel.spec.js index c475489c0605..dc3006fffd2b 100644 --- a/js/tests/unit/carousel.spec.js +++ b/js/tests/unit/carousel.spec.js @@ -2,7 +2,8 @@ import Carousel from '../../src/carousel' import EventHandler from '../../src/dom/event-handler' /** Test helpers */ -import { getFixture, clearFixture, createEvent, jQueryMock } from '../helpers/fixture' +import { clearFixture, createEvent, getFixture, jQueryMock } from '../helpers/fixture' +import * as util from '../../src/util' describe('Carousel', () => { const { Simulator, PointerEvent } = window @@ -175,8 +176,7 @@ describe('Carousel', () => { }) const spyKeydown = spyOn(carousel, '_keydown').and.callThrough() - const spyPrev = spyOn(carousel, 'prev') - const spyNext = spyOn(carousel, 'next') + const spySlide = spyOn(carousel, '_slide') const keydown = createEvent('keydown', { bubbles: true, cancelable: true }) keydown.key = 'ArrowRight' @@ -189,12 +189,10 @@ describe('Carousel', () => { input.dispatchEvent(keydown) expect(spyKeydown).toHaveBeenCalled() - expect(spyPrev).not.toHaveBeenCalled() - expect(spyNext).not.toHaveBeenCalled() + expect(spySlide).not.toHaveBeenCalled() spyKeydown.calls.reset() - spyPrev.calls.reset() - spyNext.calls.reset() + spySlide.calls.reset() Object.defineProperty(keydown, 'target', { value: textarea @@ -202,8 +200,7 @@ describe('Carousel', () => { textarea.dispatchEvent(keydown) expect(spyKeydown).toHaveBeenCalled() - expect(spyPrev).not.toHaveBeenCalled() - expect(spyNext).not.toHaveBeenCalled() + expect(spySlide).not.toHaveBeenCalled() }) it('should wrap around from end to start when wrap option is true', done => { @@ -320,7 +317,7 @@ describe('Carousel', () => { expect(carousel._addTouchEventListeners).toHaveBeenCalled() }) - it('should allow swiperight and call prev with pointer events', done => { + it('should allow swiperight and call _slide with pointer events', done => { if (!supportPointerEvent) { expect().nothing() done() @@ -348,11 +345,12 @@ describe('Carousel', () => { const item = fixtureEl.querySelector('#item') const carousel = new Carousel(carouselEl) - spyOn(carousel, 'prev').and.callThrough() + spyOn(carousel, '_slide').and.callThrough() - carouselEl.addEventListener('slid.bs.carousel', () => { + carouselEl.addEventListener('slid.bs.carousel', event => { expect(item.classList.contains('active')).toEqual(true) - expect(carousel.prev).toHaveBeenCalled() + expect(carousel._slide).toHaveBeenCalledWith('right') + expect(event.direction).toEqual('right') document.head.removeChild(stylesCarousel) delete document.documentElement.ontouchstart done() @@ -364,7 +362,7 @@ describe('Carousel', () => { }) }) - it('should allow swipeleft and call next with pointer events', done => { + it('should allow swipeleft and call previous with pointer events', done => { if (!supportPointerEvent) { expect().nothing() done() @@ -392,11 +390,12 @@ describe('Carousel', () => { const item = fixtureEl.querySelector('#item') const carousel = new Carousel(carouselEl) - spyOn(carousel, 'next').and.callThrough() + spyOn(carousel, '_slide').and.callThrough() - carouselEl.addEventListener('slid.bs.carousel', () => { + carouselEl.addEventListener('slid.bs.carousel', event => { expect(item.classList.contains('active')).toEqual(false) - expect(carousel.next).toHaveBeenCalled() + expect(carousel._slide).toHaveBeenCalledWith('left') + expect(event.direction).toEqual('left') document.head.removeChild(stylesCarousel) delete document.documentElement.ontouchstart done() @@ -409,7 +408,7 @@ describe('Carousel', () => { }) }) - it('should allow swiperight and call prev with touch events', done => { + it('should allow swiperight and call _slide with touch events', done => { Simulator.setType('touch') clearPointerEvents() document.documentElement.ontouchstart = () => {} @@ -431,11 +430,12 @@ describe('Carousel', () => { const item = fixtureEl.querySelector('#item') const carousel = new Carousel(carouselEl) - spyOn(carousel, 'prev').and.callThrough() + spyOn(carousel, '_slide').and.callThrough() - carouselEl.addEventListener('slid.bs.carousel', () => { + carouselEl.addEventListener('slid.bs.carousel', event => { expect(item.classList.contains('active')).toEqual(true) - expect(carousel.prev).toHaveBeenCalled() + expect(carousel._slide).toHaveBeenCalledWith('right') + expect(event.direction).toEqual('right') delete document.documentElement.ontouchstart restorePointerEvents() done() @@ -447,7 +447,7 @@ describe('Carousel', () => { }) }) - it('should allow swipeleft and call next with touch events', done => { + it('should allow swipeleft and call _slide with touch events', done => { Simulator.setType('touch') clearPointerEvents() document.documentElement.ontouchstart = () => {} @@ -469,11 +469,12 @@ describe('Carousel', () => { const item = fixtureEl.querySelector('#item') const carousel = new Carousel(carouselEl) - spyOn(carousel, 'next').and.callThrough() + spyOn(carousel, '_slide').and.callThrough() - carouselEl.addEventListener('slid.bs.carousel', () => { + carouselEl.addEventListener('slid.bs.carousel', event => { expect(item.classList.contains('active')).toEqual(false) - expect(carousel.next).toHaveBeenCalled() + expect(carousel._slide).toHaveBeenCalledWith('left') + expect(event.direction).toEqual('left') delete document.documentElement.ontouchstart restorePointerEvents() done() @@ -600,7 +601,7 @@ describe('Carousel', () => { const carousel = new Carousel(carouselEl, {}) const onSlide = e => { - expect(e.direction).toEqual('left') + expect(e.direction).toEqual('right') expect(e.relatedTarget.classList.contains('carousel-item')).toEqual(true) expect(e.from).toEqual(0) expect(e.to).toEqual(1) @@ -612,7 +613,7 @@ describe('Carousel', () => { } const onSlide2 = e => { - expect(e.direction).toEqual('right') + expect(e.direction).toEqual('left') done() } @@ -635,7 +636,7 @@ describe('Carousel', () => { const carousel = new Carousel(carouselEl, {}) const onSlid = e => { - expect(e.direction).toEqual('left') + expect(e.direction).toEqual('right') expect(e.relatedTarget.classList.contains('carousel-item')).toEqual(true) expect(e.from).toEqual(0) expect(e.to).toEqual(1) @@ -647,7 +648,7 @@ describe('Carousel', () => { } const onSlid2 = e => { - expect(e.direction).toEqual('right') + expect(e.direction).toEqual('left') done() } @@ -1061,6 +1062,77 @@ describe('Carousel', () => { }) }) }) + describe('rtl function', () => { + it('"_directionToOrder" and "_orderToDirection" must return the right results', () => { + fixtureEl.innerHTML = '
' + + const carouselEl = fixtureEl.querySelector('div') + const carousel = new Carousel(carouselEl, {}) + + expect(carousel._directionToOrder('left')).toEqual('prev') + expect(carousel._directionToOrder('prev')).toEqual('prev') + expect(carousel._directionToOrder('right')).toEqual('next') + expect(carousel._directionToOrder('next')).toEqual('next') + + expect(carousel._orderToDirection('next')).toEqual('right') + expect(carousel._orderToDirection('prev')).toEqual('left') + }) + + it('"_directionToOrder" and "_orderToDirection" must return the right results when rtl=true', () => { + document.documentElement.dir = 'rtl' + fixtureEl.innerHTML = '
' + + const carouselEl = fixtureEl.querySelector('div') + const carousel = new Carousel(carouselEl, {}) + expect(util.isRTL()).toEqual(true, 'rtl has to be true') + + expect(carousel._directionToOrder('left')).toEqual('next') + expect(carousel._directionToOrder('prev')).toEqual('prev') + expect(carousel._directionToOrder('right')).toEqual('prev') + expect(carousel._directionToOrder('next')).toEqual('next') + + expect(carousel._orderToDirection('next')).toEqual('left') + expect(carousel._orderToDirection('prev')).toEqual('right') + document.documentElement.dir = 'ltl' + }) + + it('"_slide" has to call _directionToOrder and "_orderToDirection"', () => { + fixtureEl.innerHTML = '
' + + const carouselEl = fixtureEl.querySelector('div') + const carousel = new Carousel(carouselEl, {}) + const spy = spyOn(carousel, '_directionToOrder').and.callThrough() + const spy2 = spyOn(carousel, '_orderToDirection').and.callThrough() + + carousel._slide('left') + expect(spy).toHaveBeenCalledWith('left') + expect(spy2).toHaveBeenCalledWith('prev') + + carousel._slide('right') + expect(spy).toHaveBeenCalledWith('right') + expect(spy2).toHaveBeenCalledWith('next') + }) + + it('"_slide" has to call "_directionToOrder" and "_orderToDirection" when rtl=true', () => { + document.documentElement.dir = 'rtl' + fixtureEl.innerHTML = '
' + + const carouselEl = fixtureEl.querySelector('div') + const carousel = new Carousel(carouselEl, {}) + const spy = spyOn(carousel, '_directionToOrder').and.callThrough() + const spy2 = spyOn(carousel, '_orderToDirection').and.callThrough() + + carousel._slide('left') + expect(spy).toHaveBeenCalledWith('left') + expect(spy2).toHaveBeenCalledWith('next') + + carousel._slide('right') + expect(spy).toHaveBeenCalledWith('right') + expect(spy2).toHaveBeenCalledWith('prev') + + document.documentElement.dir = 'ltl' + }) + }) describe('dispose', () => { it('should destroy a carousel', () => {