[Navigation Reboot] Swap the DrawerNavigator for the ResponsiveNavigator #16581
Conversation
adamgrzybowski
force-pushed
the
remove-drawer
branch
from
9367166 to
b77c421
Compare
mountiny
requested review from
parasharrajat,
mountiny,
0xmiros and
aimane-chnaif
|
Please ping us when ready for testing. |
|
I think this is kinda ready for testing but in limited scope. @adamgrzybowski would you be bale to describe what should bre ready to be tested here or give some guidance for the team once ready? Thanks! |
|
Let's add as many as possible details to the PR description bcz this is a big change and many will revert to it in the future. |
|
Hi! I think it's a good time to start some testing and look for what is missing. As you may notice, this may be a bit unusual because work is still in progress and we are assuming that there are bugs here. That's why I need your help! Those are quite big changes. It would be hard for me to check every place where something could break after this refactor and continue development in the meantime. The parts that you should focus on are related to the navigation, especially actions around the report screen and the report list. But it would be great to do some general testing. The part that you shouldn't really focus on now is navigating between screens in the right modal e.g. /settings -> /settings/profile. This part is yet to be done in other PR. There are a few issues that I am aware but I am still working on them:
Have in mind that the pattern of navigation has changed a little and now there is a possibility to push multiple report screens on the stack I see a request about the details in the PR but I guess it would be best to fill it in when removing the draft status Thanks for your help! Let me know if you need some more information. |
|
Thanks! C+ could yu try to test push notifications too? |
not locally. pretty annoying that we need to release build every time for testing push notification in android. |
…neNavigatorOnsStack
…ators in auth screen to be more consistent
|
@parasharrajat yes, thanks! |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.25-1 🚀
|
|
@mountiny @adamgrzybowski Hello! Any QA needed for this PR? |
|
@isagoico its all part of that one bigger PR, full regression is required to catch anything odd |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.25-8 🚀
|
| <SidebarScreen | ||
| navigation={navigation} | ||
| onLayout={this.trackAppStartTiming} | ||
| reportIDFromRoute={reportIDFromRoute} | ||
| /> |
There was a problem hiding this comment.
The removal of reportIDFromRoute caused a regression #20703. We added a HOC withCurrentReportId that provides currentReportId that's meant to be a replacement of reportIDFromRoute but we didn't replace all the occurrences of that old prop.
| return ( | ||
| <NavigationContainer | ||
| key={props.isSmallScreenWidth ? 'small' : 'big'} |
There was a problem hiding this comment.
This causes tons of issues when resize browser. I know this was intentional but is there alternative solution removing this line? (tagging all navigation reboot experts)
- [HOLD for payment 2023-09-07] [$1000] Form Data Doesn't Save on Web Window Resize #20404
- [HOLD #10894] Windows - Chat -Photo preview screen disappears after resizing window to full screen #20522
- [Hold App#20404] 2 factor authentiaction code dissappear when switch to small screen #20611
- [$1000] Web - The invite message disappears if resized the window screen for copy pasted message #20929
- [HOLD for payment 2023-10-13] [$500] Web - emoji picker hides on resize window #21560
- [$1000] Web - The timer to request new magic code resets every time we resize window #21616
There was a problem hiding this comment.
I don't know if we have to focus on all resizing issues. cc: @mountiny
There was a problem hiding this comment.
I don't see an alternative solution for now. Remounting is necessary to make this architecture work. Those are drawbacks of solutions that let us simplify the logic when the screen is not resized
There was a problem hiding this comment.
This is being actively worked on now by @adamgrzybowski and we will solve this by using css styles for the layout and we wont remount the navigators.
| const updateSavedNavigationStateAndLogRoute = (state) => { | ||
| navigationStateRef.current = state; | ||
| props.updateCurrentReportId(state); | ||
| parseAndLogRoute(state); |
There was a problem hiding this comment.
✋ Coming from #20825
Salute for all experts for this navigation reboot 🫡
The state can be undefined and it's crashed the app when we try to use the navigation state in other methods.
There was a problem hiding this comment.
Hi! This part has if for that now https://github.com/Expensify/App/blob/59973575da78597ccac89781b4d7117110b82078/src/libs/Navigation/NavigationRoot.js#L59C1-L66
| @@ -2,8 +2,9 @@ | |||
| import _ from 'underscore'; | |||
| import React, {forwardRef, Component} from 'react'; | |||
| import PropTypes from 'prop-types'; | |||
| import {FlatList, View} from 'react-native'; | |||
There was a problem hiding this comment.
This caused a minor type console error - #20591
| name={SCREENS.REPORT} | ||
|
|
||
| // We do it this way to avoid adding this to url | ||
| initialParams={{openOnAdminRoom: openOnAdminRoom ? 'true' : undefined}} |
There was a problem hiding this comment.
✋ Coming from #28262
This is causing a console error because the type of openOnAdminRoom is boolean or undefined.
| useFocusEffect(() => { | ||
| if (_.has(props.session, 'authToken')) { | ||
| // Pop the concierge loading page before opening the concierge report. | ||
| Navigation.goBack(); |
There was a problem hiding this comment.
This line caused a bug where the keyboard appears for a brief moment. More info on #35239
Details
This PR is a part of the navigation rework issue. Changes introduced here are related to the three issues listed below.
Motivation for changes should be included in this design DOC but I will include a short summary in the description.
Update RHP to be encapsulated in another navigator
Putting all RHP screens into one stack navigator makes it easier to implement a responsive navigator and planned navigation patterns. Fewer cases to consider.
Create the ResponsiveNavigator
This navigator has two main differences from the regular
StackNavigator.StackViewor customThreePaneViewdepending on the size of the screen.StackNavigatorand additionally ensures that there is at least one report route on the stack if the screen is big.Remove the drawer navigator
All other changes included here should be related to migrating from the
DrawerNavigatorto theResponsiveNavigatoror the new navigation patterns that this migration enables. e.g. The user can push more than one report screen on the stack.If you can't find explanations for changes in the design DOC, feel free to ask questions! I will answer or update the description.
Fixed Issues
$ #15848
$ #15851
$ #15852
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
androidweb.mov
Mobile Web - Safari
iosweb.mov
Desktop
desktop.mov
iOS
ios.mp4
Android
android.mov