Fix: back handler for Drawer #5745

parasharrajat · 2021-10-10T17:09:17Z

Details

Issues

We have to click twice on the LHN to go to any chat.
When we resize the page LHN and Report body is not correctly aligned.
Drawer does not open on Mobile Web.

Fixed Issues

$ #4211
$ #5971

Tests | QA Steps

#4211

Open LHN on Android.
navigate to a report.
Use the back button on the Android device.
You should land on LHN.
Use the back button on the Android device.
App should be closed.

#5971

There are no clear instructions for testing it but I believe this PR will fix this issue due to the reason that we are using upgraded Drawer Lib. The new version has removed transformation from the View for the permanent drawer which means it should remain static.

Follow the reproduction steps mentioned on the issue.
Or, Try to resize the Browser window multiple times to observe that the layout does not break.

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

parasharrajat · 2021-10-10T17:17:23Z

One issue that I found is when we open the Workspace Settings route, it does not open the Card page at the click of the Card link. This is known to me and I am fixing it #5131 here.

Beamanator

Tests are looking great!

One weird Android thing I noticed: When viewing the app in the simulator, I have to press twice on any report to navigate to it. Once I navigate to a report I can close it and navigate to it again with one click, but if I close and want to navigate to a new report I have to click that report twice :( But it looks like that's also happening on main so not caused by this 👍

Still can't merge b/c n6 hold

parasharrajat · 2021-10-11T16:03:45Z

The PR which I mentioned in the above comment is solving this.

Beamanator · 2021-10-11T16:06:36Z

@parasharrajat Sorry, which PR are you thinking will fix the issue I mentioned?

parasharrajat · 2021-10-11T16:16:42Z

#5131

Beamanator · 2021-10-11T16:19:58Z

Ok I trust you 😅 your comment here & the test steps in #5131 didn't really look super related to what I mentioned, but it's fine, I know this PR didn't cause the issue 👍

parasharrajat · 2021-10-11T16:24:37Z

Well, don't trust me on this. Your issue is indeed different. Didn't get a good look from Cellphone.

I will check that later with your reproduction steps.

Beamanator · 2021-10-11T16:30:59Z

Aah ok 😆 I'll post in #expensify-open-source to see if others saw the same issue recently

parasharrajat · 2021-10-18T16:52:27Z

@Beamanator Do not merge it yet. I have to test something.

parasharrajat · 2021-10-18T21:58:06Z

All right.

It will cause two new issues.

We have to click twice on the LHN to go to any chat.
When we resize the page LHN and Report body are not correctly aligned.

@Beamanator What should I do now?

Beamanator · 2021-10-19T23:33:37Z

@parasharrajat I just tested this fork again and noticed:

Needing to click twice on the LHN wasn't happening for me! That's a good sign 👍
If you resize the page so whole page is the report body, then expand so you should see the LHN again, the LHN disappears
- Tested this on main, I didn't see this happen. I don't see how this could have happened due to adding backBehavior="none", could it have been caused by something else in the libraries you updated in this PR?
- I removed backBehavior="none" on your fork and re-tested, saw the same thing. Can you check if this has been reported in react-navigation?

parasharrajat · 2021-10-19T23:41:41Z

I looked at the diff between the versions.

There are a couple of changes which can be directly the cause of our issues.

History for the Drawer is changed. # 1
There are a couple of changes for the drawer-type parmanant. react-navigation/react-navigation@2a88d0d

I think the first can be fixed directly in our app. We are managing the state ourself and thus we could be wrong somewhere.

For 2, I am yet to analyze the changes and their effect.

parasharrajat · 2021-10-20T02:15:08Z

OK, @Beamanator I have to open another issue with R-navigation for 2 on the above list 👨 🔫 . It's clearly a regression from the above commit.

My suggestion

I think the work here for 4211 and 5027 is done. So those two milestone should be considered completed (Upwork).
Now we have two more issues which is blocking this PR.
I can try to fix the first one. A new milestone as this is a new issue.
For point 2, I have to open another issue and provide reproduction and cooperate with maintainers to fix that. This is additional work for me and you know how tedious it could be, so I request separate milestone for this as well.

We can hold the payment for all the above until this PR and other issues are fixed.

What do you suggest as the best path forward?

Beamanator · 2021-10-20T20:52:54Z

Great ideas @parasharrajat , I definitely like this path to move forward, and I agree that you've already completed a few milestones and we should open a few more. Here's what I'm thinking (almost exactly what you mentioned, plus one milestone you missed, and I want to confirm with Rory about #5027)

Milestones / tasks completed

[HOLD for payment 2021-12-06] The Android hardware back button does not minimise the app when pressed on the LHN/home screen #4211 ($1000 - price for this job was increased here)
mWeb - Refreshing the LHN page opens chat with recent user #5027 - @roryabraham can you confirm if this PR fixes this issue? Also can you confirm that the payout for that issue is $1000 (price last increased here)? (I already added the n6-hold bonus below)
- Confirmed, this PR fixes that issue
Allow disabling BackHandler subscription in DrawerView react-navigation/react-navigation#9936 (We agreed here) that this is a separate job for $250
This PR was submitted before the n6-hold ended so should get the $250 bonus

New milestones / tasks

Submit issue in react-navigation to fix point 2 from this comment

About point 1 from this comment, I don't see how this is related to changes in this PR, if you can convince me then I'll absolutely be on board it's a new issue & new job

parasharrajat · 2021-10-21T01:09:21Z

Well. @Beamanator There are a few things that you are missing.

[HOLD for payment 2021-12-06] The Android hardware back button does not minimise the app when pressed on the LHN/home screen #4211 => Last priced at $1000 [HOLD for payment 2021-12-06] The Android hardware back button does not minimise the app when pressed on the LHN/home screen #4211 (comment).
mWeb - Refreshing the LHN page opens chat with recent user #5027 => Last Priced at $1000.

But I am still investigating the 2. I am looking at the code to see if this could a bug with our configuration so I will complete the repro to compare.

For 1, I believe it's related as it was not happening earlier and only after these changes. I have checked the changes and the lib author has changes regarding drawer history which are causing our logic to fail.

App/src/libs/Navigation/CustomActions.js

Lines 75 to 87 in 550dfe6

    
                   const screenRoute = {type: 'route', name: screenName}; 
        
                   const history = [...(state.history || [])].map(() => screenRoute); 
        
                   history.push(screenRoute); 
        
                   return CommonActions.reset({ 
        
                       ...state, 
        
                       routes: [{ 
        
                           name: screenName, 
        
                           params, 
        
                       }], 
        
                       history, 
        
                   }); 
        
               }; 
        
           }

Let me debug it further.

parasharrajat · 2021-10-21T17:27:16Z

If you resize the page so the whole page is the report body, then expand so you should see the LHN again, the LHN disappears.

@Beamanator I just fixed point 2. testing in Native Devices.

Working on an easy one now 1.

parasharrajat · 2021-10-21T18:11:11Z

@Beamanator You would be happy to know. I have fixed both issues and We are ready to light this up.

We have to click twice on the LHN to go to any chat.

Analysis

I looked at the code for the drawer router and come to know that it usages history to determine the Drawer Status. But when we are resetting the state, we are not sending any history for drawer status. Thus it takes the Default status.
So on the first click, the report loads but the drawer is not closed.
On Second, it just takes to the report screen via closing the drawer.

parasharrajat · 2021-10-27T00:28:01Z

Updated.

marcaaron

Sorry, I know this has been a long PR, but these changes seem to have caused a not so great regression with navigating to a report

Navigate to a report
Press back button in header
Search for a user
Select chat
Land on LHN instead of chat 💥

marcaaron · 2021-10-27T00:50:21Z

src/libs/Navigation/AppNavigator/DrawerNavigator/index.js

+        // Drawer's implementaion is buggy. Modern implementaion does not work on mobile web and legacy breaks the layout on Desktop while resizing.
+        // For some reason, drawer never closes when opening the Drawer screen on mobile web.
+        // So we will switch between implementations based on the device width.


Suggested change

// Drawer's implementaion is buggy. Modern implementaion does not work on mobile web and legacy breaks the layout on Desktop while resizing.

// For some reason, drawer never closes when opening the Drawer screen on mobile web.

// So we will switch between implementations based on the device width.

// On mobile web, the non legacy version of the DrawerNavigator breaks the layout on Desktop while resizing

// and the drawer gets stuck in an open state when navigating

Is there any issue we are tracking for this?

No. I will create one.

No yet, I will create one in our repo.

marcaaron · 2021-10-27T00:53:41Z

package.json

    "babel-plugin-transform-remove-console": "^6.9.4",
+    "babel-polyfill": "^6.26.0",


Just curious, what needed babel-polyfill?

I followed https://docs.swmansion.com/react-native-reanimated/docs/fundamentals/web-support. I don't know the consequences of removing this.

parasharrajat · 2021-10-27T02:55:05Z

I kind of know what is causing this. I will look at it asap.

parasharrajat · 2021-10-29T01:33:00Z

Updated. Ready for review.

parasharrajat · 2021-10-29T23:59:22Z

cc: @marcaaron @Beamanator

marcaaron

Nice working good now thanks!

marcaaron · 2021-10-30T00:43:49Z

All yours @Beamanator.

Beamanator

Overall the changes look great, the back button on android is working great 👍 and resizing the window in desktop & web have no regressions 👍

The only 🤷 things are:

refreshing on web / desktop when you have a chat open, takes you back to the LHN
- As mentioned in earlier comments, you're planning to fix this in another PR, right?
the comment below
- If you disagree with me / I'm confused, no change is necessary :D

src/libs/Navigation/AppNavigator/DrawerNavigator/index.js

parasharrajat · 2021-10-30T15:30:04Z

refreshing on web / desktop when you have a chat open, takes you back to the LHN
As mentioned in earlier comments, you're planning to fix this in another PR, right?

Unfortunately, we won't be solving the #5027 (comment) anymore. But yeah It would have been solved separately.

src/libs/Navigation/AppNavigator/DrawerNavigator/index.js

parasharrajat · 2021-11-01T13:03:16Z

@Beamanator Comment updated. Ready for merge.

Beamanator

Thanks for all of this 💪

OSBotify · 2021-11-01T15:35:43Z

🚀 Deployed to staging by @Beamanator in version: 1.1.11-6 🚀

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

kidroca · 2021-11-02T18:10:13Z

package.json

-    "react-native-reanimated": "^2.3.0-alpha.1",
+    "react-native-reanimated": "^2.3.0-beta.3",


Hey @Beamanator @parasharrajat this change here seems to have broken the Attachment Picker for me on iOS: https://expensify.slack.com/archives/C01GTK53T8Q/p1635874753343600

Hey, @kidroca this is PR is reverted now. Thanks for notifying me.

OSBotify · 2021-11-02T23:36:30Z

🚀 Deployed to production by @yuwenmemon in version: 1.1.12-3 🚀

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

fix: back handler for Drawer

bf5b355

parasharrajat requested a review from a team as a code owner October 10, 2021 17:09

MelvinBot requested review from Beamanator and removed request for a team October 10, 2021 17:09

Beamanator assigned parasharrajat Oct 11, 2021

Beamanator previously approved these changes Oct 11, 2021

View reviewed changes

Pull latest changes

3753c9d

Pull latest changes

2fd9753

Merge branch 'main' of github.com:Expensify/Expensify.cash into fix-back

d438612

Beamanator self-requested a review October 20, 2021 20:53

fix: Drawer on web via upgrading the reanimated to latest

58f6d1e

parasharrajat dismissed Beamanator’s stale review via 58f6d1e October 21, 2021 17:34

parasharrajat mentioned this pull request Oct 21, 2021

Desktop - LHN is not displayed when reopening app from dock #5971

Closed

fix: Drawer navigation

0c1a2a8

Changes

b50b783

parasharrajat dismissed roryabraham’s stale review via b50b783 October 26, 2021 23:13

marcaaron requested changes Oct 27, 2021

View reviewed changes

parasharrajat mentioned this pull request Oct 28, 2021

[HOLD for payment 2021-12-06] [Hold for payment on 25th Nov] [$1000] Android/iOS - Changing orientation breaks layout #5591

Closed

parasharrajat added 2 commits October 29, 2021 06:49

fix: Routing issue

080ce7e

Changes

9fe64ea

parasharrajat requested review from marcaaron and Beamanator October 29, 2021 01:33

marcaaron previously approved these changes Oct 30, 2021

View reviewed changes

Beamanator reviewed Oct 30, 2021

View reviewed changes

src/libs/Navigation/AppNavigator/DrawerNavigator/index.js Outdated Show resolved Hide resolved

Beamanator reviewed Oct 30, 2021

View reviewed changes

src/libs/Navigation/AppNavigator/DrawerNavigator/index.js Outdated Show resolved Hide resolved

comment improved

05aa313

parasharrajat dismissed marcaaron’s stale review via 05aa313 November 1, 2021 13:02

Beamanator self-requested a review November 1, 2021 14:18

Beamanator approved these changes Nov 1, 2021

View reviewed changes

Beamanator merged commit f2fb01a into Expensify:main Nov 1, 2021

OSBotify mentioned this pull request Nov 1, 2021

Deploy Checklist: New Expensify 2021-10-29 #6127

Closed

23 tasks

marcaaron mentioned this pull request Nov 1, 2021

Revert "Fix: back handler for Drawer" #6161

Merged

kidroca reviewed Nov 2, 2021

View reviewed changes

botify mentioned this pull request Nov 2, 2021

[HOLD for payment 2021-12-06] The Android hardware back button does not minimise the app when pressed on the LHN/home screen #4211

Closed

5 tasks

parasharrajat mentioned this pull request Nov 17, 2021

Fix back handler for Android #6295

Merged

5 tasks

		"babel-plugin-transform-remove-console": "^6.9.4",
		"babel-polyfill": "^6.26.0",

		"react-native-reanimated": "^2.3.0-alpha.1",
		"react-native-reanimated": "^2.3.0-beta.3",

Fix: back handler for Drawer #5745

Fix: back handler for Drawer #5745

Conversation

parasharrajat commented Oct 10, 2021 • edited by roryabraham Loading

Details

Issues

Fixed Issues

Tests | QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

parasharrajat commented Oct 10, 2021

Beamanator left a comment

Choose a reason for hiding this comment

parasharrajat commented Oct 11, 2021

Beamanator commented Oct 11, 2021

parasharrajat commented Oct 11, 2021

Beamanator commented Oct 11, 2021

parasharrajat commented Oct 11, 2021

Beamanator commented Oct 11, 2021

parasharrajat commented Oct 18, 2021

parasharrajat commented Oct 18, 2021 • edited Loading

Beamanator commented Oct 19, 2021 • edited Loading

parasharrajat commented Oct 19, 2021 • edited Loading

parasharrajat commented Oct 20, 2021 • edited Loading

Beamanator commented Oct 20, 2021 • edited Loading

parasharrajat commented Oct 21, 2021 • edited Loading

parasharrajat commented Oct 21, 2021 • edited Loading

parasharrajat commented Oct 21, 2021 • edited Loading

Analysis

parasharrajat commented Oct 27, 2021

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented Oct 27, 2021

parasharrajat commented Oct 29, 2021

parasharrajat commented Oct 29, 2021

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron commented Oct 30, 2021

Beamanator left a comment

Choose a reason for hiding this comment

parasharrajat commented Oct 30, 2021 • edited Loading

parasharrajat commented Nov 1, 2021 • edited Loading

Beamanator left a comment

Choose a reason for hiding this comment

OSBotify commented Nov 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Nov 2, 2021

parasharrajat commented Oct 10, 2021 •

edited by roryabraham

Loading

parasharrajat commented Oct 18, 2021 •

edited

Loading

Beamanator commented Oct 19, 2021 •

edited

Loading

parasharrajat commented Oct 19, 2021 •

edited

Loading

parasharrajat commented Oct 20, 2021 •

edited

Loading

Beamanator commented Oct 20, 2021 •

edited

Loading

parasharrajat commented Oct 21, 2021 •

edited

Loading

parasharrajat commented Oct 21, 2021 •

edited

Loading

parasharrajat commented Oct 21, 2021 •

edited

Loading

parasharrajat commented Oct 30, 2021 •

edited

Loading

parasharrajat commented Nov 1, 2021 •

edited

Loading