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

Add rtl support with dynamic right & left properties #70

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

iamsoorena
Copy link

This PR resolves this issue.
I've checked result of this PR with both RTL and LTR apps.
It works as expected in both situations.

  • LTR(default):

  • RTL:

tooltip.right = tooltip.right === 0 ? tooltip.right + MARGIN : tooltip.right;
tooltip.maxWidth = layout.width - tooltip.right - MARGIN;
arrow.right = tooltip.right + MARGIN;
tooltip[end] = Math.max(layout.width - (obj.left + obj.width), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason you changed the syntax, instead of using tooltip.end?

Copy link
Owner

Choose a reason for hiding this comment

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

The bracket notation is for computed keys. tooltip.end = 1; results in { end: 1 } whereas const end = 'left'; tooltip[end] = 1 gives { left: 1 }, unless I misunderstood your question.

@@ -78,23 +83,23 @@ class ViewMask extends Component<Props, State> {
style={[
styles.overlayRectangle,
{
right: leftOverlayRight,
[end]: leftOverlayRight,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change of syntax, again? How do the brackets help?

Choose a reason for hiding this comment

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

@RichardLitt
This syntax reaches the variable instead of the stable key, so the 'end' variable may switch depending on the LTR or RTL orientations.

Copy link
Contributor

@RichardLitt RichardLitt left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, @iamsoorena! Made a few small comments.

@iamsoorena
Copy link
Author

iamsoorena commented Sep 5, 2018

Hi @RichardLitt ,
React Native Allegedly 😊 has support for end and start, you can find more info about it here.
But I'm currently working on a RTL app. And I know that this feature is not well implemented in react native. It simply doesn't work as expected.
So I defined my own start, end.

const rtl = I18nManager.isRTL;
const start = rtl ? 'right' : 'left';
const end = rtl ? 'left' : 'right';

So If I used end: leftOverlayRight, without brackets, React-native would have replaced end with It's own definition, BUT I want end to be strictly set as left in RTL apps.


We can refactor usage of [end] [start] in the future when React-Native resolved this issue. I'll keep an eye for that. 😊

@mohebifar
Copy link
Owner

@iamsoorena Thanks! I'm on a vacation right now. I'll test and review this next week.

@mohebifar
Copy link
Owner

@iamsoorena Sorry for the delay on reviewing this. This looks really good! Thank you. I'm just curious if it is possible to programmatically switch the direction. For example, when changing the language within the app's settings (English -> Farsi -> English). I couldn't find any function for switching back to LTR on I18nManager. I18nManager.forceRTL(false) also does not seem to work.

@mohebifar
Copy link
Owner

This looks good to me. I would just like to know if the programmatic change of direction is a possibility. In that case, we would have to move the RTL check (Line 48-50) into the _animateMove method.

@iamsoorena
Copy link
Author

@iamsoorena Sorry for the delay on reviewing this. This looks really good! Thank you. I'm just curious if it is possible to programmatically switch the direction. For example, when changing the language within the app's settings (English -> Farsi -> English). I couldn't find any function for switching back to LTR on I18nManager. I18nManager.forceRTL(false) also does not seem to work.

@mohebifar I've not used or implemented this feature before. I don't know about this. 😀

@iamsoorena
Copy link
Author

This looks good to me. I would just like to know if the programmatic change of direction is a possibility. In that case, we would have to move the RTL check (Line 48-50) into the _animateMove method.

@mohebifar As far as I know, using I18nManager.forceRTL(false/true) should do the trick, but I've not used it before.

@RichardLitt RichardLitt requested review from mohebifar and removed request for mohebifar November 7, 2018 17:48
@mohebifar
Copy link
Owner

Seems that #82 includes these changes as well as the onPress thing. It would be better if the PRs only introduce one new feature or bug fix. But for now, we can just close this one, and follow up the rest in #82 and merge both functionalities at once.

@iamsoorena
Copy link
Author

@mohebifar First I think you mean #71.
Since I thought #71 would be merged long time ago, I just worked on that branch to avoid conflicts in the future, But I don't have a problem with separating PRs.

@RichardLitt
Copy link
Contributor

@iamsoorena Thanks for letting us know, @iamsoorena. Sorry about the delay - we don't have unlimited time to work on this repository. :)

@iamsoorena
Copy link
Author

@RichardLitt no problem.

@mohebifar
Copy link
Owner

@iamsoorena Just wanted to follow up with you to see if you want still would like to:

  1. Make the change to check the direction dynamically in the measurement code. (For example, for apps that support switching the language in the settings menu)
  2. Keep out the changes available in this PR from Add onPress support in the middle of steps #71 so that Add onPress support in the middle of steps #71 will only contain changes related to the onPress functionality.
    Thanks!

@iamsoorena
Copy link
Author

@mohebifar once again I'm sorry for my slow responds.
critically busy on a project.
I'll let you know ASAP.

@ab-sami
Copy link

ab-sami commented Oct 24, 2021

Can someone please merge this PR. It's look good to me as well.
It's really urgent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem in RTL apps
5 participants