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.

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

Merged
merged 9 commits into from
Mar 17, 2021

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Jan 28, 2021

It fixes rtl next/previous functionality,
it separates directions from order, and transfers all the rtl and direction responsibility on the _slide method

Preview: https://deploy-preview-32913--twbs-bootstrap.netlify.app/docs/5.0/components/carousel/

@GeoSot GeoSot requested a review from a team as a code owner January 28, 2021 01:21
@XhmikosR
Copy link
Member

XhmikosR commented Feb 4, 2021

Looks good to me but this reminds me that we don't have tests for this code.

Also, we already have next() and prev(), perhaps we could leverage them instead of adding new functions?

@GeoSot
Copy link
Member Author

GeoSot commented Feb 5, 2021

I did the change you propose
Cause of RTL support I would prefer to keep left/right against next/previous

@GeoSot
Copy link
Member Author

GeoSot commented Feb 5, 2021

if vertical carousel #32988 is going to be merged, I think the left/right is better to be independent of the next/prev

@XhmikosR
Copy link
Member

XhmikosR commented Feb 5, 2021

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?

@GeoSot
Copy link
Member Author

GeoSot commented Feb 8, 2021

@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

@rohit2sharma95
Copy link
Collaborator

rohit2sharma95 commented Feb 14, 2021

@GeoSot You can make isRTL a function in the util if you need to check the dir in the runtime 🤔
Edit:- but that is not needed in real

@XhmikosR XhmikosR changed the title Move carousel common code to reusable functions Fix carousel RTL and refactor code Feb 15, 2021
@GeoSot GeoSot changed the title Fix carousel RTL and refactor code Fix carousel RTL and refactor code, fix rtl swipe issues Feb 15, 2021
@aqeelat
Copy link
Contributor

aqeelat commented Feb 15, 2021

Hi, I made #33076 before noticing this PR. Would appreciate it if you take a look.

@rohit2sharma95
Copy link
Collaborator

I think #32446 should be merged first 🤔

@ffoodd ffoodd mentioned this pull request Feb 16, 2021
20 tasks
@GeoSot
Copy link
Member Author

GeoSot commented Feb 16, 2021

@rohit2sharma95 I added the checks.
You will find two new methods directionToOrder and orderToDirection
Both of them, maybe would have to be extracted in future to support other modules with right/left directions

@GeoSot GeoSot force-pushed the right-left-methods-on-carousel branch 2 times, most recently from 3cbf4e1 to 0db8787 Compare March 2, 2021 21:19
@GeoSot GeoSot force-pushed the right-left-methods-on-carousel branch from 0db8787 to 6bffe0f Compare March 15, 2021 21:28
@GeoSot GeoSot force-pushed the right-left-methods-on-carousel branch from 62d0c57 to 76d2222 Compare March 16, 2021 20:09
Copy link
Collaborator

@rohit2sharma95 rohit2sharma95 left a 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 🙂

js/tests/unit/carousel.spec.js Outdated Show resolved Hide resolved
@mdo
Copy link
Member

mdo commented Mar 17, 2021

@XhmikosR Will leave for you to squash as you like :)

@XhmikosR XhmikosR merged commit b9f3090 into twbs:main Mar 17, 2021
@GeoSot GeoSot deleted the right-left-methods-on-carousel branch March 18, 2021 21:42
reviewtypo3org pushed a commit to TYPO3/typo3 that referenced this pull request Jun 14, 2021
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 <a.fernandez@scripting-base.de>
Tested-by: core-ci <typo3@b13.com>
Tested-by: Nikita Hovratov <nikita.h@live.de>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Reviewed-by: Nikita Hovratov <nikita.h@live.de>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
TYPO3IncTeam pushed a commit to TYPO3-CMS/backend that referenced this pull request Jun 14, 2021
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 <a.fernandez@scripting-base.de>
Tested-by: core-ci <typo3@b13.com>
Tested-by: Nikita Hovratov <nikita.h@live.de>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Reviewed-by: Nikita Hovratov <nikita.h@live.de>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants