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.

Sign up for GitHub

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

Fix carousel sliding direction in RTL pages #33076

Closed
wants to merge 6 commits into from
Closed

Fix carousel sliding direction in RTL pages #33076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
392aed7
fixed movement direction to reflect off of the ltr version.
aqeelat Feb 13, 2021
1afd445
1- Added unit tests
aqeelat Feb 15, 2021
aa54a72
Merge branch 'main' of https://github.com/twbs/bootstrap into rtl-car…
aqeelat Feb 15, 2021
48980ec
Merge branch 'main' into rtl-carousel
aqeelat Feb 15, 2021
098708b
deleted commented code and fixed typo
aqeelat Feb 15, 2021
4c6cb1d
Merge remote-tracking branch 'origin/rtl-carousel' into rtl-carousel
aqeelat Feb 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 22 additions & 10 deletions js/src/carousel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ class Carousel extends BaseComponent {
this._touchSupported = 'ontouchstart' in document.documentElement || navigator.maxTouchPoints > 0
this._pointerEvent = Boolean(window.PointerEvent)

this.isRTL = this._element.dir === 'rtl' || isRTL
Copy link
Member

Choose a reason for hiding this comment

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

Why is the new check needed? Or why doesn't the previous one suffice?

BTW, I have a draft PR around to switch isRTL to a function so that it's tree-shaken, but I doubt it's related.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to add it so that I can produce the unit tests I was asked for. Then I realized it might be helpful in some cases.

isRTL only checks if the tag has dir=rtl attribute. This means that you can't have an RTL carousel otherwise. One use case: in the documentation for bootstrap itself on how to make a page rtl. The page itself could be in English, but the carousel could be in Arabic.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we support such a case in v5 @ffoodd ?

Copy link
Collaborator

@rohit2sharma95 rohit2sharma95 Feb 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that case is not supported yet. And this check is then redundant 🙂

Copy link
Member

Choose a reason for hiding this comment

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

@aqeelat: I merged #32446. Your use case makes sense, but I thought it wasn't supported.

That being said, it seems it's sort of supported https://getbootstrap.com/docs/5.0/getting-started/rtl/#ltr-and-rtl-at-the-same-time, so after #32913 or in #32913, we could refactor the isRTL function to check for an element's direction and or document.documentElement.dir.

We just need to make sure we have JS tests to cover these changes

this._addEventListeners()
}

Expand Down Expand Up @@ -241,19 +242,19 @@ class Carousel extends BaseComponent {
}

_handleSwipe() {
const absDeltax = Math.abs(this.touchDeltaX)
const absDeltaX = Math.abs(this.touchDeltaX)

if (absDeltax <= SWIPE_THRESHOLD) {
if (absDeltaX <= SWIPE_THRESHOLD) {
return
}

const direction = absDeltax / this.touchDeltaX
const direction = absDeltaX / this.touchDeltaX

this.touchDeltaX = 0

// swipe left
if (direction > 0) {
if (isRTL) {
if (this.isRTL) {
this.next()
} else {
this.prev()
Expand All @@ -262,7 +263,7 @@ class Carousel extends BaseComponent {

// swipe right
if (direction < 0) {
if (isRTL) {
if (this.isRTL) {
this.prev()
} else {
this.next()
Expand Down Expand Up @@ -350,14 +351,14 @@ class Carousel extends BaseComponent {

if (event.key === ARROW_LEFT_KEY) {
event.preventDefault()
if (isRTL) {
if (this.isRTL) {
this.next()
} else {
this.prev()
}
} else if (event.key === ARROW_RIGHT_KEY) {
event.preventDefault()
if (isRTL) {
if (this.isRTL) {
this.prev()
} else {
this.next()
Expand Down Expand Up @@ -449,9 +450,20 @@ class Carousel extends BaseComponent {
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
// the logic behind the next three constants is a shorthand for:
// if (this.isRTL) return direction === DIRECTION_NEXT ? option 2 : option 1
// else return direction === DIRECTION_NEXT ? option 1 : option 2
// +-------+------------------------------+----------+
// | isRTL | direction === DIRECTION_NEXT | |
// +-------+------------------------------+----------+
// | T | T | Option 2 |
// | T | F | Option 1 |
// | F | T | Option 1 |
// | F | F | Option 2 |
// +-------+------------------------------+----------+
const directionalClassName = this.isRTL === (direction === DIRECTION_NEXT) ? CLASS_NAME_END : CLASS_NAME_START
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I like these, I feel like it's less readable...

const orderClassName = this.isRTL === (direction === DIRECTION_NEXT) ? CLASS_NAME_PREV : CLASS_NAME_NEXT
const eventDirectionName = this.isRTL === (direction === DIRECTION_NEXT) ? DIRECTION_RIGHT : DIRECTION_LEFT

if (nextElement && nextElement.classList.contains(CLASS_NAME_ACTIVE)) {
this._isSliding = false
Expand Down
70 changes: 70 additions & 0 deletions js/tests/unit/carousel.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,6 +689,76 @@ describe('Carousel', () => {

carousel.next()
})

it('RTL should fire slide event with: direction, relatedTarget, from and to', done => {
fixtureEl.innerHTML = [
'<div dir="rtl" id="myCarousel" class="carousel slide">',
' <div class="carousel-inner">',
' <div class="carousel-item active">item 1</div>',
' <div id="secondItem" class="carousel-item">item 2</div>',
' <div class="carousel-item">item 3</div>',
' </div>',
'</div>'
].join('')

const carouselEl = fixtureEl.querySelector('#myCarousel')
const carousel = new Carousel(carouselEl, {})

const onSlide = e => {
expect(e.direction).toEqual('right')
expect(e.relatedTarget.classList.contains('carousel-item')).toEqual(true)
expect(e.from).toEqual(0)
expect(e.to).toEqual(1)

carouselEl.removeEventListener('slide.bs.carousel', onSlide)
carouselEl.addEventListener('slide.bs.carousel', onSlide2)

carousel.prev()
}

const onSlide2 = e => {
expect(e.direction).toEqual('left')
done()
}

carouselEl.addEventListener('slide.bs.carousel', onSlide)
carousel.next()
})

it('RTL should fire slid event with: direction, relatedTarget, from and to', done => {
fixtureEl.innerHTML = [
'<div dir="rtl" id="myCarousel" class="carousel slide">',
' <div class="carousel-inner">',
' <div class="carousel-item active">item 1</div>',
' <div id="secondItem" class="carousel-item">item 2</div>',
' <div class="carousel-item">item 3</div>',
' </div>',
'</div>'
].join('')

const carouselEl = fixtureEl.querySelector('#myCarousel')
const carousel = new Carousel(carouselEl, {})

const onSlid = e => {
expect(e.direction).toEqual('right')
expect(e.relatedTarget.classList.contains('carousel-item')).toEqual(true)
expect(e.from).toEqual(0)
expect(e.to).toEqual(1)

carouselEl.removeEventListener('slid.bs.carousel', onSlid)
carouselEl.addEventListener('slid.bs.carousel', onSlid2)

carousel.prev()
}

const onSlid2 = e => {
expect(e.direction).toEqual('left')
done()
}

carouselEl.addEventListener('slid.bs.carousel', onSlid)
carousel.next()
})
})

describe('nextWhenVisible', () => {
Expand Down