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

[HOLD for payment 2022-03-17] App changes positions of Ui elements when phone language RTL - Reported by Ahmed sherif #7476

Closed
mvtglobally opened this issue Jan 31, 2022 · 28 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@mvtglobally
Copy link

mvtglobally commented Jan 31, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Set phone language to any RTL
  2. Open app
  3. Login & Browse

Expected Result:

App UI should be properly displayed

Actual Result:

UI is distorted, some elements overlap

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

  • iOS
  • Android

Version Number: 1.1.33-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Screenshot_٢٠٢٢٠١٢٤-١٨٣٦٥٠_New Expensify
Screenshot_٢٠٢٢٠١٢٤-١٨٣٥٤٨_New Expensify

Expensify/Expensify Issue URL:
Issue reported by: Ahmed sherif
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1643042672341700

Upwork Job https://www.upwork.com/jobs/~019ba4918a8529eaf2
View all open jobs on GitHub

@mvtglobally mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Jan 31, 2022
@MelvinBot
Copy link

Triggered auto assignment to @stephanieelliott (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jan 31, 2022
@stephanieelliott stephanieelliott removed their assignment Jan 31, 2022
@MelvinBot
Copy link

Triggered auto assignment to @TomatoToaster (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@TomatoToaster TomatoToaster added the External Added to denote the issue can be worked on by a contributor label Jan 31, 2022
@MelvinBot
Copy link

Triggered auto assignment to @kevinksullivan (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@kevinksullivan
Copy link
Contributor

@MelvinBot
Copy link

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 31, 2022
@MelvinBot
Copy link

Triggered auto assignment to @marcaaron (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@TehseenJawed
Copy link

That's the simple styling problem that we can sort out with the help of flexbox properties. I can resolve these issues and will be happy to give you support for maintaining your application front-end and backend. Please let me know if you have some more questions in mind. We can arrange a call to discuss it further.

@parasharrajat
Copy link
Member

I don't think we are ready to add RTL support ATM. let me discuss this and get back. ✋

@Daneyl
Copy link

Daneyl commented Feb 1, 2022

I will be using react-native-i18n library to resolve the RTL layout issue. This library will check for RTL/ LTR language and returns boolean which we will use to render UI smoothly. I have already worked over this issue before on a website. You can check here.
http://omenkul.com/

The above website also has mobile apps (Android & iOS), however, they are not live. I have the source code for it on my repository.

@ahmdshrif
Copy link
Contributor

ahmdshrif commented Feb 1, 2022

@parasharrajat I think all we need is to force the app to work LTR since we only support LTR

will add this

 I18nManager.allowRTL(false);
  I18nManager.forceRTL(false);
  I18nManager.swapLeftAndRightInRTL(false);

and will need to add more steps on android native.

  I18nUtil sharedI18nUtilInstance = I18nUtil.getInstance();
  sharedI18nUtilInstance.allowRTL(getApplicationContext(), false);

@parasharrajat
Copy link
Member

So based on the discussion, we would like to block the RTL mode for the time being until we wish to support RTL.

Please submit updated proposals.

@ahmdshrif Where did I18nManager come from?

@ahmdshrif
Copy link
Contributor

ahmdshrif commented Feb 1, 2022

It come from React-native .

Ok i will update it

@ahmdshrif
Copy link
Contributor

ahmdshrif commented Feb 1, 2022

https://reactnative.dev/blog/2016/08/19/right-to-left-support-for-react-native-apps

React native have this blog post to support rtl . And we use the same to not support rtl .

I see here he add the native config to support rtl. So i will add it also may be will need this config on some android or ios versions. (It work fine with me without need to them)

So adding this for android

in MainActivity.java

I18nUtil sharedI18nUtilInstance = I18nUtil.getInstance();
sharedI18nUtilInstance.allowRTL(context, false);

In AndroidManifest.xml

And adding android:supportsRtl="false" 

And for ios .

// in AppDelegate.m
[[RCTI18nUtil sharedInstance] allowRTL:false];

@parasharrajat
Copy link
Member

// in MainActivity.java
I18nUtil sharedI18nUtilInstance = I18nUtil.getInstance();
sharedI18nUtilInstance.allowRTL(context, false);

I think we don't need this part after adding android:supportsRtl="false"

Apart from it, @ahmdshrif 's proposal looks good to me.

cc: @marcaaron

🎀 👀 🎀 C+ reviewed

@ahmdshrif
Copy link
Contributor

@parasharrajat can we start pr here?

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 4, 2022
@MelvinBot
Copy link

📣 @ahmdshrif You have been assigned to this job by @marcaaron!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@marcaaron
Copy link
Contributor

@ahmdshrif @parasharrajat can we get a status update here?

@MelvinBot MelvinBot removed the Overdue label Feb 14, 2022
@parasharrajat
Copy link
Member

PR is in progress.. Updates are being made.

@marcaaron
Copy link
Contributor

This is in review now and close to merge.

@MelvinBot MelvinBot removed the Overdue label Feb 23, 2022
@marcaaron marcaaron added the Reviewing Has a PR in review label Feb 23, 2022
@ahmdshrif ahmdshrif mentioned this issue Feb 25, 2022
7 tasks
@marcaaron
Copy link
Contributor

Just leaving a comment here to let everyone know there is a second PR related to this issue -> #7922 (comment)

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 10, 2022
@botify botify changed the title App changes positions of Ui elements when phone language RTL - Reported by Ahmed sherif [HOLD for payment 2022-03-17] App changes positions of Ui elements when phone language RTL - Reported by Ahmed sherif Mar 10, 2022
@botify
Copy link

botify commented Mar 10, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.41-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-03-17. 🎊

@botify botify removed the Weekly KSv2 label Mar 17, 2022
@MelvinBot MelvinBot added the Daily KSv2 label Mar 17, 2022
@ahmdshrif
Copy link
Contributor

Hi @kevinksullivan i think this should be pay now .

@kevinksullivan
Copy link
Contributor

Hi @ahmdshrif sorry I missed this and thanks for the reminder. The total payment should be $250, not $500, correct? I see you requested $500 and want to be sure I'm giving you the right amount.

@parasharrajat can you accept the hire for C+ ?

@ahmdshrif
Copy link
Contributor

ahmdshrif commented Mar 29, 2022

@kevinksullivan no problem.. i request 500 because i report and fix this issue .

@ahmdshrif
Copy link
Contributor

@kevinksullivan here is the slack report.
https://expensify.slack.com/archives/C01GTK53T8Q/p1643042672341700?thread_ts=1643042672.341700&cid=C01GTK53T8Q

@kevinksullivan
Copy link
Contributor

All set. Thanks @ahmdshrif @parasharrajat !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests