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 2021-12-06] [Hold for payment on 25th Nov] [$1000] Android/iOS - Changing orientation breaks layout #5591

Closed
isagoico opened this issue Sep 29, 2021 · 48 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented Sep 29, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel! https://www.upwork.com/jobs/~0130cf8bfe4742fbde


Action Performed:

  1. On an Android with a big screen
  2. Open the app
  3. On LHN, change the orientation to landscape

Expected Result:

Layout should not break

Actual Result:

Layout is broken after changing the orientation to landscape.

Workaround:

Closing and reopening the app removes the issue.

Platform:

Where is this issue occurring?

  • Android

Version Number: 1.1.3-3

Reproducible in staging?: yes
Reproducible in production?: yes

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

layout.mp4

Expensify/Expensify Issue URL:

Issue reported by: @Santhosh-Sellavel
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1631730111364600

View all open jobs on GitHub

@isagoico isagoico added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Sep 29, 2021
@MelvinBot
Copy link

Triggered auto assignment to @laurenreidexpensify (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 Sep 29, 2021
@MelvinBot
Copy link

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

@isagoico
Copy link
Author

I was able to reproduce this issue in a Galaxy A52 (6.5-inch screen size)

@MonilBhavsar
Copy link
Contributor

MonilBhavsar commented Sep 30, 2021

Was able to reproduce the reported issue on Android simulator and iOS device...

Screenshot_1632993247

... But looking more into screen orientation of our app, looks like we need to improve it a lot. In the app, on login page in landscape mode, the highlighted part of screen is responsive, Trying to scroll down by sideways makes one feel like app is unresponsive...

IMG_9001 2

...We also need to improve transition when changing from landscape to portrait and vice versa.

RPReplay_Final1632986507.MP4

@MonilBhavsar MonilBhavsar added Improvement Item broken or needs improvement. External Added to denote the issue can be worked on by a contributor labels Sep 30, 2021
@laurenreidexpensify
Copy link
Contributor

Having some issues getting this on Upwork, will upload tomorrow

@MonilBhavsar MonilBhavsar changed the title Android - Changing orientation breaks layout Android/iOS - Changing orientation breaks layout Oct 1, 2021
@laurenreidexpensify
Copy link
Contributor

@MelvinBot MelvinBot added Weekly KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors and removed Daily KSv2 labels Oct 1, 2021
@MelvinBot
Copy link

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

@laurenreidexpensify
Copy link
Contributor

@MonilBhavsar @Julesssss you can decide between two of you who wants to stay assigned 🙃

@MonilBhavsar
Copy link
Contributor

Oh, I should have unassigned myself from this one after adding external label.

@MonilBhavsar MonilBhavsar removed their assignment Oct 1, 2021
@Julesssss
Copy link
Contributor

Honestly, I think we should just block landscape mode for now. It can be a lot of work to support a rarely used feature.

@MonilBhavsar
Copy link
Contributor

MonilBhavsar commented Oct 1, 2021

👍 I see slack doesn't have this feature too

@roryabraham
Copy link
Contributor

I do think we should just fix this – though we should take it one problem at a time. It doesn't need to be a top priority for us, but when we find something that's very broken with orientation change, we can fix it.

@Julesssss
Copy link
Contributor

Although the team mostly disagrees that landscape mode is an important feature for mobile, valid counterpoints have been made here. We support iPad, and landscape mode is arguably the best way to consume the app. So we should fix landscape bugs.

@kadiealexander
Copy link
Contributor

Please refer to this post for updated information on the n6 hold, we've raised the bonus to $250 for all issues where a PR is created before the N6 hold is lifted.

@parasharrajat
Copy link
Member

parasharrajat commented Nov 17, 2021

@Julesssss

Proposal

After research and testing on all platforms, I found that there is glitch in Drawer animation which we can't solve by adjusting any of its props. When we switch between slide to permanent drawer type the translation style is not removed either from Screen or sidebar(it's random). I am also able to reproduce this on the web when we switch between mobile and web resolution. I am working on a PR where I updated the version of R-Navigation and its severity got increased.

I have a solution for this.

as

key={`BaseDrawerNavigator${props.isSmallScreenWidth}`} //There is only one instance of BaseDrawerNavigator so no duplicate keys

we can cause a re-render of the drawer on orientation change and which will recalculate and fix it. The cost of it is the same as switching reports (Does not affect UX).

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 17, 2021
@MelvinBot
Copy link

📣 @parasharrajat You have been assigned to this job by @Julesssss!
Please apply to this job in Upwork and let us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@Julesssss
Copy link
Contributor

Smart fix, the re-render is definitely a change worth making.

Lauren is off for a few days, but please feel free to make a start on the PR if that suits you @parasharrajat.

@laurenreidexpensify please hire @parasharrajat when you are back.

@MelvinBot MelvinBot removed the Overdue label Nov 17, 2021
@kadiealexander
Copy link
Contributor

kadiealexander commented Nov 17, 2021

Just helping Lauren out, @parasharrajat could you please submit in Upwork so I can hire you? Thanks!

@parasharrajat
Copy link
Member

@kadiealexander Did.

@Julesssss
Copy link
Contributor

Merged, awaiting payment.

@Julesssss Julesssss changed the title [$1000] Android/iOS - Changing orientation breaks layout [Hold for payment on 25th Nov] [$1000] Android/iOS - Changing orientation breaks layout Nov 18, 2021
@laurenreidexpensify
Copy link
Contributor

Thanks Julesbot 🤖

@laurenreidexpensify laurenreidexpensify added Daily KSv2 and removed Weekly KSv2 labels Nov 25, 2021
@laurenreidexpensify
Copy link
Contributor

I completely missed hiring Rajat in Upwork on this one, so making a daily so I make sure to pay as soon as he's accepted the offer

@MelvinBot MelvinBot removed the Overdue label Nov 25, 2021
@parasharrajat
Copy link
Member

Just did @laurenreidexpensify.

@laurenreidexpensify
Copy link
Contributor

@parasharrajat paid!

@Santhosh-Sellavel I'll issue you a $250 bonus now

@laurenreidexpensify
Copy link
Contributor

Nearly finished with payment for Santhosh, will close after that

@laurenreidexpensify
Copy link
Contributor

Everyone has been paid - closing 👍🏽

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Nov 29, 2021
@botify botify changed the title [Hold for payment on 25th Nov] [$1000] Android/iOS - Changing orientation breaks layout [HOLD for payment 2021-12-06] [Hold for payment on 25th Nov] [$1000] Android/iOS - Changing orientation breaks layout Nov 29, 2021
@botify
Copy link

botify commented Nov 29, 2021

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.16-10 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 2021-12-06. 🎊

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 Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Weekly KSv2
Projects
None yet
Development

No branches or pull requests