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.

Sign up for GitHub

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

Jump to bottom

Fix back handler for Android #6295

Merged
merged 6 commits into from
Nov 22, 2021
Merged

Fix back handler for Android #6295

merged 6 commits into from
Nov 22, 2021

Conversation

parasharrajat
Copy link
Member

Details

Fixed Issues

$ #4211

Tests | QA Steps

  1. Open LHN on Android.
  2. navigate to a report.
  3. Use the back button on the Android device.
  4. You should land on LHN.
  5. Use the back button on the Android device.
  6. App should be closed.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android (affected)

Screenshots

Web

Mobile Web

Desktop

iOS

Android

output_file.mp4

@parasharrajat parasharrajat requested a review from a team as a code owner November 14, 2021 17:25
@MelvinBot MelvinBot requested review from Beamanator and removed request for a team November 14, 2021 17:25
@parasharrajat
Copy link
Member Author

@Beamanator I have fixed the IOS freezing issues which caused the last PR to be reverted. All of the changes are same as per last PR and just adjusted the package version for Reanimated so that it does not cause the freeze this time.

Please test it and this should be ready for merge.

@parasharrajat
Copy link
Member Author

parasharrajat commented Nov 16, 2021 •
edited
Loading

Wait, I think I need to clean this up and a few tweaks are remaining.

@parasharrajat parasharrajat changed the title Fix back handler for Android [HOLD] Fix back handler for Android Nov 16, 2021
@parasharrajat
Copy link
Member Author

parasharrajat commented Nov 17, 2021 •
edited
Loading

@Beamanator
Ok, I have done the testing on each platform, and the only issue that is remaining and for which I updated the Reanimated package in the previous PR is point 2 on #5745 (comment).

But I found out that this issue already exists in #5591. Just increasing the package version increased its severity.

On #5591, when we change the orientation, it switches the drawer from side to permanent, and the exact same thing happens on point 2 where we move from Mobile view to web.

but but but I have a solution for that.

remember in this previous PR we were using the userLegacyImplementation prop to remount the drawer when screening switches between mobile and web. We can do the same by using the key prop(we don't have userLegacyImplementation) now it fixes that issue. But I will do that separately into different PR as a solution for that issue. Proposed on that issue as well. So after that is merged I will proceed with this PR.
image

@parasharrajat parasharrajat changed the title [HOLD] Fix back handler for Android Fix back handler for Android Nov 18, 2021
@parasharrajat
Copy link
Member Author

This is ready.
PR for #5591 is #6356.

@Beamanator
Copy link
Contributor

Wow nice work here, and thanks for linking to the other PR where you fixed the orientation issue! I'll review this today :)

Beamanator
Beamanator reviewed Nov 22, 2021
View reviewed changes
src/libs/Navigation/AppNavigator/BaseDrawerNavigator.js
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions';
import styles, {getNavigationDrawerStyle, getNavigationDrawerType} from '../../../styles/styles';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why move this down here? I kinda like it better above withWindowDimensions to make imports slightly more alphabetical

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, I'd rather get this merged haha

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, what I am 9 minutes late.... 🐼

Copy link
Contributor

Choose a reason for hiding this comment

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

lol it's fine - i thought you would be later :D

Beamanator
Beamanator approved these changes Nov 22, 2021
View reviewed changes
Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Looks awesome, fantastic work here dude 👍

I also verified it didn't cause either of these regressions that the last one caused

@Beamanator Beamanator merged commit f866f8b into Expensify:main Nov 22, 2021
@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 OSBotify mentioned this pull request Nov 22, 2021
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Beamanator in version: 1.1.15-18 🚀

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

@botify botify mentioned this pull request Nov 29, 2021
Closed
5 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.16-10 🚀

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

@parasharrajat parasharrajat deleted the fix-back branch November 20, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Beamanator Beamanator Beamanator approved these changes

Assignees

@parasharrajat parasharrajat

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@parasharrajat @Beamanator @OSBotify