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

fix first load #7922

Merged
merged 4 commits into from
Feb 28, 2022
Merged

fix first load #7922

merged 4 commits into from
Feb 28, 2022

Conversation

ahmdshrif
Copy link
Contributor

@ahmdshrif ahmdshrif commented Feb 25, 2022

Details

Fixed Issues

$ #7476

Tests

  • Verify that no errors appear in the JS console

QA Steps

  1. Set phone language to any RTL ( Settings -> Languages & input -> Languages ) chose Arabic for example
  2. remove the app and reinstall it.
  3. Open the app
  4. Login & Browse
  5. make sure the UI element is in its normal position
  6. make sure the normal text also does not align to right instead of left
  7. got to text input and use English keyboard (RTL language not supported)
  8. make sure text input aligns to left
  9. test on android and ios . and flow all previous steps.
  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2022-02-07 at 2 22 50 AM

Mobile Web

Screenshot_20220207-211827_Chrome.jpg

Desktop

Screen Shot 2022-02-07 at 2 30 40 AM

iOS

Screen Shot 2022-02-07 at 2 02 02 AM
55b-d8534034caed.png)

Screen.Recording.2022-02-13.at.3.42.50.PM.mov

Android

Screen Shot 2022-02-07 at 1 45 17 AM

@ahmdshrif ahmdshrif requested a review from a team as a code owner February 25, 2022 19:51
@MelvinBot MelvinBot requested review from marcaaron and parasharrajat and removed request for a team February 25, 2022 19:51
@marcaaron
Copy link
Contributor

What's the context for this PR? Was there a regression somewhere?

@ahmdshrif ahmdshrif changed the title fix android first load fix first load Feb 25, 2022
@ahmdshrif
Copy link
Contributor Author

ahmdshrif commented Feb 25, 2022

I find the edge case here when you install the app for the first time app does not read the RTL config from js until you reload it. and will work after that very fine.

the js config we add before. work fine with us because the app was installed.

just add this native config line that fix this case.

i know this issue takes a while but we should solve this.

@marcaaron
Copy link
Contributor

Got it thanks!

Copy link
Member

@parasharrajat parasharrajat 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 submitting a PR @ahmdshrif Appreciate it.

ios/NewExpensify/AppDelegate.m Outdated Show resolved Hide resolved
ahmdshrif and others added 2 commits February 26, 2022 10:20
Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>
Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM.

cc: @marcaaron

🎀 👀 🎀 C+ reviewed

@marcaaron marcaaron merged commit 72daa6f into Expensify:main Feb 28, 2022
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

OSBotify commented Mar 1, 2022

🚀 Deployed to staging by @marcaaron in version: 1.1.41-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 failure ❌

@OSBotify
Copy link
Contributor

OSBotify commented Mar 2, 2022

🚀 Deployed to staging by @marcaaron in version: 1.1.41-0 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 failure ❌

@mvtglobally
Copy link

Team is having a bit hard time validating this PR on Desktop/Web as it requires device language change and introducing navigation issues to flip language back as options shift.
We validated mWeb/IOS and Android. It would be great if someone can validate internally Desktop/Web

@ahmdshrif
Copy link
Contributor Author

HI, @mvtglobally I will suggest here to change the language for the app and browser only from setting here.

Screen Shot 2022-03-07 at 10 05 30 AM

@francoisl
Copy link
Contributor

Hi @mvtglobally, were you able to test with the suggestion just above?

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @francoisl in version: 1.1.41-6 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

6 participants