-
-
Notifications
You must be signed in to change notification settings - Fork 78.9k
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.
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 RTL and refactor code, fix rtl swipe issues #32913
Conversation
Looks good to me but this reminds me that we don't have tests for this code. Also, we already have |
I did the change you propose |
if vertical carousel #32988 is going to be merged, I think the |
I'm not sure about #32988; it lacks tests docs and everything. Either way, can you please add a few tests to ensure that when isRTL is true, the right this is done? |
@rohit2sharma95 any help how to mock the isRTL import? I was not able to find any way, so for now I used it as a component method |
@GeoSot You can make |
Hi, I made #33076 before noticing this PR. Would appreciate it if you take a look. |
I think #32446 should be merged first 🤔 |
@rohit2sharma95 I added the checks. |
3cbf4e1
to
0db8787
Compare
0db8787
to
6bffe0f
Compare
62d0c57
to
76d2222
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may need to update the spec names 🙂
@XhmikosR Will leave for you to squash as you like :) |
Bootstrap beta3 introduced a regression, which reversed the slide direction. This therefore broke our implementation, since we are using an event listener, evaluating the direction. Because we were not aware of this being "just" a regression instead of an intended API change, we switched the direction in our event listener in #94037, too. In the final 5.0 release, we updated to in #94089, the regression has been fixed, breaking our event listener once again. This is fixed by restoring the initial and correct direction check. Related bootstrap PRs: - twbs/bootstrap#32913 - twbs/bootstrap#33499 Resolves: #94338 Releases: master Change-Id: I0bc4dc2dfd37935fc43e04f311bd850916c31004 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/69486 Tested-by: Andreas Fernandez <[email protected]> Tested-by: core-ci <[email protected]> Tested-by: Nikita Hovratov <[email protected]> Tested-by: Christian Kuhn <[email protected]> Reviewed-by: Andreas Fernandez <[email protected]> Reviewed-by: Nikita Hovratov <[email protected]> Reviewed-by: Christian Kuhn <[email protected]>
Bootstrap beta3 introduced a regression, which reversed the slide direction. This therefore broke our implementation, since we are using an event listener, evaluating the direction. Because we were not aware of this being "just" a regression instead of an intended API change, we switched the direction in our event listener in #94037, too. In the final 5.0 release, we updated to in #94089, the regression has been fixed, breaking our event listener once again. This is fixed by restoring the initial and correct direction check. Related bootstrap PRs: - twbs/bootstrap#32913 - twbs/bootstrap#33499 Resolves: #94338 Releases: master Change-Id: I0bc4dc2dfd37935fc43e04f311bd850916c31004 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/69486 Tested-by: Andreas Fernandez <[email protected]> Tested-by: core-ci <[email protected]> Tested-by: Nikita Hovratov <[email protected]> Tested-by: Christian Kuhn <[email protected]> Reviewed-by: Andreas Fernandez <[email protected]> Reviewed-by: Nikita Hovratov <[email protected]> Reviewed-by: Christian Kuhn <[email protected]>
It fixes rtl next/previous functionality,
it separates directions from order, and transfers all the rtl and direction responsibility on the
_slide
methodPreview: https://deploy-preview-32913--twbs-bootstrap.netlify.app/docs/5.0/components/carousel/