Skip to content

Fix/page carousel RTL android #1612

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

Jump to bottom
Merged
merged 5 commits into from
Oct 24, 2021
Merged

Fix/page carousel RTL android #1612

merged 5 commits into from
Oct 24, 2021

Conversation

ethanshar
Copy link
Collaborator

@ethanshar ethanshar commented Oct 19, 2021 •
edited
Loading

Description

Suggest going over the commit by order

  • First, I centralized all cases we set the currentPage and added a method that does that
  • Second, I made some fixes in PageCarousel, I noticed the scrollToIndex has some redundant calls
  • Last two commits - I comment some parts of that don't seem relevant anymore and fixed the actual RTL issue

WOAUILIB-2153

Changelog

TabController.PageCarousel - Fix RTL issue on Android

@ethanshar ethanshar requested a review from M-i-k-e-l October 19, 2021 10:24
@ethanshar ethanshar changed the title Fix/page carousel rtl android Fix/page carousel RTL android Oct 19, 2021
@ethanshar ethanshar added the Important for Next Release PR that must be included in the release version label Oct 20, 2021
M-i-k-e-l
M-i-k-e-l reviewed Oct 20, 2021
View reviewed changes
@ethanshar ethanshar requested a review from M-i-k-e-l October 24, 2021 05:53
M-i-k-e-l
M-i-k-e-l approved these changes Oct 24, 2021
View reviewed changes
Copy link
Collaborator

@M-i-k-e-l M-i-k-e-l left a comment

Choose a reason for hiding this comment

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

Approved, added a comment\question

src/components/tabController/PageCarousel.tsx
@@ -75,7 +101,8 @@ function PageCarousel({...props}) {
showsHorizontalScrollIndicator={false}
onScroll={scrollHandler}
scrollEventThrottle={16}
contentOffset={contentOffset} // iOS only
contentOffset={initialOffset} // iOS only
onLayout={scrollToInitial} // Android only
Copy link
Collaborator

Choose a reason for hiding this comment

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

Android with RTL has a visible change of page with initialIndex; according to the docs, contentOffset should work for Android, but it doesn't, should we create a ticket to check-up on it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's a very old issue..
not sure when they will even support that.
I wouldn't try to hard to solve that.. at least for now

@ethanshar ethanshar merged commit f59c958 into master Oct 24, 2021
@ethanshar ethanshar deleted the fix/PageCarouselRTLAndroid branch April 18, 2022 09:01
@snyk-bot snyk-bot mentioned this pull request Feb 10, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@M-i-k-e-l M-i-k-e-l M-i-k-e-l approved these changes

Assignees

@M-i-k-e-l M-i-k-e-l

Labels
Important for Next Release PR that must be included in the release version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ethanshar @M-i-k-e-l