Upgrade react-navigation to 6.x #3078

tugbadogan · 2021-05-22T15:13:49Z

Details

Upgrade react-navigation to 6.x and reanimated library to 2.x to resolve issues caused by legacy drawer and animation implementations.

Fixed Issues

Fixes #2180 and #2447

Tests

Tested navigating between chats and other pages like settings, search and made sure they work as expected.

On mobile devices changed device orientation and observed that the components are positioned properly.

QA Steps

Login to expensify.cash
Navigate between chats and access all other pages (Search, settings, user profiles)
Check if chats are visible on the background when modal pages are opened
Verify that navigation and page animations are the same as the production

Try to reproduce these issues as instructed in the issue descriptions

Tested On

Screenshots

Web

Screen.Recording.2021-06-06.at.22.52.33-3.mov

Mobile Web

Screen.Recording.2021-06-06.at.22.56.58.mov

Desktop

Untitled.mp4

iOS

Screen.Recording.2021-05-22.at.17.06.10-2.mov

Android

Screen.Recording.2021-05-22.at.17.34.32-3.mov

src/libs/Navigation/linkTo.js

src/pages/home/report/ReportActionCompose.js

src/pages/home/report/ReportActionsView.js

tugbadogan · 2021-05-23T16:14:34Z

Only problem we have right now is that opening modals in web and desktop hides chat window in the background. It shows gray background instead. I'm trying to resolve it now.

Jag96

Looks like there are conflicts, any luck fixing the backgrounds on web and desktop?

tugbadogan · 2021-05-26T22:44:41Z

Yes @Jag96 after couple of hours of debugging, I was finally able to resolve that issue by adding detachInactiveScreens={false} to RootStack.Navigator. I'm going to update the screen recordings soon.

marcaaron

I have only tested web so far but it looks kind of broken

marcaaron · 2021-05-27T00:55:38Z

src/libs/Navigation/AppNavigator/AuthScreens.js

@@ -178,6 +178,7 @@ class AuthScreens extends React.Component {
                // a header will briefly open and close the keyboard and crash Android.
                // eslint-disable-next-line react/jsx-props-no-multi-spaces
                keyboardHandlingEnabled={false}
+                detachInactiveScreens={false}


Hmm I am not sure if we want this. Doesn't this mean that we are missing out on memory savings?

https://reactnavigation.org/docs/stack-navigator/#detachinactivescreens

When I don't set this prop, chat window view in the background doesn't work for modal views on web. We just have gray background.

Interesting, @tugbadogan how did you determine that setting that prop to false would fix the issue? Just wondering if there are alternate solutions since this has the side effect @marcaaron mentioned above.

I found this by accident while looking at the documentation. But, we don't need this anymore as I fixed the issue by upgrading react-native-screens package version to >3.0.0 as required by react-navigation 6.x and used presentation: 'transparentModal' for modal screen options.

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js

marcaaron · 2021-05-27T01:04:57Z

there are also warnings in the console

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js

tugbadogan · 2021-06-03T00:31:03Z

Hey @marcaaron , I fixed the lint errors and empty chat issue on web, can you do another review?

… prop

tugbadogan · 2021-06-06T22:16:00Z

I updated screen recordings for web, desktop and mobile web. I also added QA steps.

marcaaron

This is testing good for me on web now.

Although, the animations look slightly different on web. I'm not sure if we want to roll with them or make sure they stay the same as they are on production/staging? Any thoughts on this @Jag96 @shawnborton?

After / New
https://user-images.githubusercontent.com/32969087/121085080-232e2c00-c77d-11eb-8679-2e52d29d287f.mp4

Before / Old
https://user-images.githubusercontent.com/32969087/121085092-25908600-c77d-11eb-8fd9-372417b7fe5a.mp4

marcaaron · 2021-06-07T20:31:01Z

src/libs/Navigation/AppNavigator/AuthScreens.js

@@ -166,6 +166,7 @@ class AuthScreens extends React.Component {
            animationEnabled: true,
            gestureDirection: 'horizontal',
            cardOverlayEnabled: true,
+            presentation: 'transparentModal',


Why do we need this? Can we add some comment maybe?

This option is required to make previous screen visible underneath the modal screen
https://reactnavigation.org/docs/6.x/stack-navigator#transparent-modals

Adding a comment to the code now 👍

marcaaron · 2021-06-07T20:31:58Z

src/styles/styles.js

            borderColor: themeColors.border,
        }
        : {
-            height: '100%',
+            height: windowHeight,


I'm confused about this change. Why does this not work with 100% but does work with an absolute value?

I don't know TBH. This might be related to some components we're using for layout management (maybe ScreenWrapper)

Ok cool no worries I just like to understand the changes, but if they work they work.

Jag96

Although, the animations look slightly different on web. I'm not sure if we want to roll with them or make sure they stay the same as they are on production/staging?

I see the difference as well, it looks like it now behaves the same way as iOS does. I don't see any issue with using the updated animations. @shawnborton is OOO this week so going to tag @michelle-thompson to weigh in here in case there's some context I'm missing.

Otherwise, this looks good and works well for me 👍

marcaaron · 2021-06-08T00:41:16Z

I see the difference as well, it looks like it now behaves the same way as iOS does. I don't see any issue with using the updated animations.

Nice I feel the same. They look more consistent so it's a net positive just unexpected 🙃

michelle-thompson · 2021-06-08T23:16:21Z

Looks good to me as well 👍

Jag96

LGTM

OSBotify · 2021-06-09T00:05:27Z

🚀 Deployed to staging in version: 1.0.65-2🚀

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

OSBotify · 2021-06-15T16:54:25Z

🚀 Deployed to production in version: 1.0.68-4🚀

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

tugbadogan and others added 3 commits May 11, 2021 18:21

Merge branch 'Expensify:main' into master

d9035f6

Merge branch 'Expensify:main' into master

85dfdbf

Upgrade react-navigation to 6.x

e76d11d

tugbadogan requested a review from a team as a code owner May 22, 2021 15:13

MelvinBot requested review from Jag96 and removed request for a team May 22, 2021 15:14

tugbadogan changed the title ~~[DRAFT] 00Upgrade react-navigation to 6.x~~ [DRAFT] Upgrade react-navigation to 6.x May 22, 2021

tugbadogan commented May 22, 2021

View reviewed changes

src/libs/Navigation/linkTo.js Outdated Show resolved Hide resolved

tugbadogan commented May 22, 2021

View reviewed changes

src/pages/home/report/ReportActionCompose.js Outdated Show resolved Hide resolved

tugbadogan commented May 22, 2021

View reviewed changes

src/pages/home/report/ReportActionsView.js Outdated Show resolved Hide resolved

Resolved exceptions

2852e12

tugbadogan and others added 2 commits May 23, 2021 17:15

Fix android and web exceptions

89b7caa

Merge branch 'Expensify:main' into master

a75fe58

tugbadogan changed the title ~~[DRAFT] Upgrade react-navigation to 6.x~~ Upgrade react-navigation to 6.x May 25, 2021

Jag96 reviewed May 26, 2021

View reviewed changes

tugbadogan and others added 4 commits May 26, 2021 23:34

Merge branch 'Expensify:main' into master

2841b8f

Fix missing previous screen on web and desktop

536b839

Merge branch 'master' into tugbadogan-upgrade-react-navigation-drawer

5319798

Fix Podfile.lock

a729c8a

Jag96 requested a review from marcaaron May 27, 2021 00:19

marcaaron requested changes May 27, 2021

View reviewed changes

marcaaron reviewed May 27, 2021

View reviewed changes

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js Outdated Show resolved Hide resolved

tugbadogan and others added 5 commits June 2, 2021 22:13

Merge branch 'Expensify:main' into master

dcb0cbd

Merge branch 'master' into tugbadogan-upgrade-react-navigation-drawer

b434435

Fix lint errors

0ac0c70

Remove redundant comment

e613d11

Fix empty chat and scroll issue on web

37b3591

tugbadogan mentioned this pull request Jun 3, 2021

iPad - Chat - Chats are not accessible on iPad in Portrait mode (Pay on June 15th) #2447

Closed

tugbadogan added 2 commits June 4, 2021 00:17

Upgrade react-native-screens version and remove detachInactiveScreens…

c0766f5

… prop

Update Podfile.lock

9c067fd

marcaaron previously approved these changes Jun 7, 2021

View reviewed changes

marcaaron assigned tugbadogan Jun 7, 2021

Add comment to presentation transparentModal

aa81556

tugbadogan dismissed marcaaron’s stale review via aa81556 June 7, 2021 21:37

Jag96 reviewed Jun 8, 2021

View reviewed changes

marcaaron self-requested a review June 8, 2021 00:42

marcaaron approved these changes Jun 8, 2021

View reviewed changes

Jag96 approved these changes Jun 8, 2021

View reviewed changes

Jag96 merged commit b115b43 into Expensify:main Jun 8, 2021

OSBotify mentioned this pull request Jun 8, 2021

Deploy Checklist: Expensify.cash 2021-06-08 #3450

Closed

49 tasks

Julesssss mentioned this pull request Jun 14, 2021

[HOLD for payment 21 June] iOS - Attachment picker flickers and unable to select for choose document #3543

Closed

tugbadogan deleted the tugbadogan-upgrade-react-navigation-drawer branch July 10, 2021 16:41

VickyStash mentioned this pull request Dec 5, 2023

[TS migration] Migrate 'AppNavigator' lib to TypeScript #29157

Merged

58 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade react-navigation to 6.x #3078

Upgrade react-navigation to 6.x #3078

tugbadogan commented May 22, 2021 •

edited

Loading

tugbadogan commented May 23, 2021

Jag96 left a comment

tugbadogan commented May 26, 2021

marcaaron left a comment

marcaaron May 27, 2021

tugbadogan Jun 3, 2021

Jag96 Jun 3, 2021

tugbadogan Jun 3, 2021

marcaaron commented May 27, 2021

tugbadogan commented Jun 3, 2021

tugbadogan commented Jun 6, 2021

marcaaron left a comment •

edited

Loading

marcaaron Jun 7, 2021

tugbadogan Jun 7, 2021

marcaaron Jun 7, 2021

tugbadogan Jun 7, 2021

marcaaron Jun 8, 2021

Jag96 left a comment

marcaaron commented Jun 8, 2021

michelle-thompson commented Jun 8, 2021

Jag96 left a comment

OSBotify commented Jun 9, 2021

OSBotify commented Jun 15, 2021

Upgrade react-navigation to 6.x #3078

Upgrade react-navigation to 6.x #3078

Conversation

tugbadogan commented May 22, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

tugbadogan commented May 23, 2021

Jag96 left a comment

Choose a reason for hiding this comment

tugbadogan commented May 26, 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

marcaaron commented May 27, 2021

tugbadogan commented Jun 3, 2021

tugbadogan commented Jun 6, 2021

marcaaron left a comment • edited Loading

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

Jag96 left a comment

Choose a reason for hiding this comment

marcaaron commented Jun 8, 2021

michelle-thompson commented Jun 8, 2021

Jag96 left a comment

Choose a reason for hiding this comment

OSBotify commented Jun 9, 2021

OSBotify commented Jun 15, 2021

tugbadogan commented May 22, 2021 •

edited

Loading

marcaaron left a comment •

edited

Loading