Skip to content

WheelPicker + SectionsWheelPicker - RTL support #2354

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

Merged
merged 9 commits into from
Dec 13, 2022
Merged

Conversation

lidord-wix
Copy link
Contributor

@lidord-wix lidord-wix commented Nov 30, 2022

Description

Add RTL support (and disableRTL support) to WheelPicker and SectionsWheelPicker.
Also added an example to the SectionsWheelPicker example screen to show this new feature on RTL. (and to make sure it works 😇)
This PR is an infrastructure to support WOAUILIB-3027

Changelog

Add RTL support (and disableRTL support) to WheelPicker and SectionsWheelPicker.

@lidord-wix lidord-wix added the Important for Next Release PR that must be included in the release version label Dec 6, 2022
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.

Sorry, the example screen doesn't make sense to me, why do we cancel the translation here?

I've also noticed that the spacings between the wheel and label are different in Android and iOS (this happens when changing the device's language)

Comment on lines +122 to +124
const shouldDisableRTL = useMemo(() => {
return Constants.isRTL && disableRTL;
}, [disableRTL]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a little inconsistent that here there's a shouldDisableRTL and there isn't one in SectionsWheelPicker.
I'll probably be weird to have this everywhere, so probably without is better (unless there's a reason to it).
Also, if you want to keep this then perhaps rename the prop in Item to something else.

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 added shouldDisableRTL to SectionsWheelPicker as well.. the idea is to let the user pass the disableRTL prop always and not only for RTL languages (we should take care of the RTL check).
And I don't really like the idea of renaming the prop in Item, I prefer to keep all the props with the same name - disableRTL

@lidord-wix lidord-wix requested a review from M-i-k-e-l December 12, 2022 08:14
@lidord-wix
Copy link
Contributor Author

@M-i-k-e-l I added an example to the screen like in the WheelPickerScreen but kept the previous example (mainly for us, to see how it changes and not changes in RTL).
let me know what you think

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.

Approving with these comments:

  1. I'd probably put the Disable RTL next to the bottom example since it's more relevant to that.
  2. If you scroll the hours in the dialog you'll see a small adjustment in the minutes side, this happens because there is a difference in the width of 1 and 10, I think this is a real bug in our component because if a user will want to do something like this they will have this bug (I did not find a way to solve it) - perhaps we want to open a ticket for this.

@lidord-wix
Copy link
Contributor Author

@M-i-k-e-l I updated the screen with Diana's design, feel free to go over it if you want and merge it if not :)

@M-i-k-e-l
Copy link
Collaborator

@M-i-k-e-l I updated the screen with Diana's design, feel free to go over it if you want and merge it if not :)

Why not both? :)
The button is disabled when not in RTL 👏

@M-i-k-e-l M-i-k-e-l merged commit 23d9ae3 into master Dec 13, 2022
@lidord-wix
Copy link
Contributor Author

@M-i-k-e-l I updated the screen with Diana's design, feel free to go over it if you want and merge it if not :)

Why not both? :) The button is disabled when not in RTL 👏

All the credit to Diana :)

@lidord-wix lidord-wix deleted the feat/WheelPicker_rtl branch January 23, 2023 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Important for Next Release PR that must be included in the release version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants