-
Notifications
You must be signed in to change notification settings - Fork 734
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
Conversation
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.
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)
const shouldDisableRTL = useMemo(() => { | ||
return Constants.isRTL && disableRTL; | ||
}, [disableRTL]); |
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.
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.
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.
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
@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). |
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.
Approving with these comments:
- I'd probably put the
Disable RTL
next to the bottom example since it's more relevant to that. - 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
and10
, 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.
@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? :) |
All the credit to Diana :) |
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.