Fix carousel sliding direction in RTL pages #33076
Conversation
|
Could you provide a reduced test case please, or describe exactly what's wrong? Because as of v5.0.0-beta2, they don't slide in the same direction: Am I missing something? |
|
When the slides are cycling, they both move in the same direction, which should not be the case. Before the fix, the RTL version would slide in the same way as the LTR. |
|
We need tests, please add some tests. |
|
I'm working on the tests but I'm having trouble coming up with a way to mock index.isRTL. Any suggestions? |
2- made it so that rtl carousels can be used in ltl pages, by using a localized dir='rtl' tag. This could be replaced by a designated class.
|
I'd stick with But JS is not my specialty. @rohit2sharma95 any opinion on this? |
| // | F | T | Option 1 | | ||
| // | F | F | Option 2 | | ||
| // +-------+------------------------------+----------+ | ||
| const directionalClassName = this.isRTL === (direction === DIRECTION_NEXT) ? CLASS_NAME_END : CLASS_NAME_START |
There was a problem hiding this comment.
Not sure I like these, I feel like it's less readable...
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I don't think we support such a case in v5 @ffoodd ?
There was a problem hiding this comment.
Yes, that case is not supported yet. And this check is then redundant 🙂
There was a problem hiding this comment.
@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
|
We also have #32913 FYI |
|
@rohit2sharma95 @ffoodd I think that #32913 is better to understand and it should have the same fix. Please have a look there and let's close this one. @aqeelat thanks for the PR, regardless :) |
|
Closing in favor of #32913. We can revisit later if needed. |
Both LTR and RTL carousels slid in the same direction.
Fixed and tested.