-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix: navigation for exisiting report screen #5131
Changes from all commits
c006880
28533f6
89ed687
aabd93f
5cd8450
432b542
9cee8d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,42 @@ | ||
import {CommonActions, StackActions, DrawerActions} from '@react-navigation/native'; | ||
import lodashGet from 'lodash/get'; | ||
|
||
/** | ||
* Go back to the Main Drawer | ||
* @param {Object} navigationRef | ||
*/ | ||
function navigateBackToRootDrawer(navigationRef) { | ||
let isLeavingNestedDrawerNavigator = false; | ||
|
||
// This should take us to the first view of the modal's stack navigator | ||
navigationRef.current.dispatch((state) => { | ||
// If this is a nested drawer navigator then we pop the screen and | ||
// prevent calling goBack() as it's default behavior is to toggle open the active drawer | ||
if (state.type === 'drawer') { | ||
isLeavingNestedDrawerNavigator = true; | ||
return StackActions.pop(); | ||
} | ||
|
||
// If there are multiple routes then we can pop back to the first route | ||
if (state.routes.length > 1) { | ||
return StackActions.popToTop(); | ||
} | ||
|
||
// Otherwise, we are already on the last page of a modal so just do nothing here as goBack() will navigate us | ||
// back to the screen we were on before we opened the modal. | ||
return StackActions.pop(0); | ||
}); | ||
|
||
if (isLeavingNestedDrawerNavigator) { | ||
return; | ||
} | ||
|
||
// Navigate back to where we were before we launched the modal | ||
if (navigationRef.current.canGoBack()) { | ||
navigationRef.current.goBack(); | ||
} | ||
} | ||
|
||
/** | ||
* In order to create the desired browser navigation behavior on web and mobile web we need to replace any | ||
* type: 'drawer' routes with a type: 'route' so that when pushing to a report screen we never show the | ||
|
@@ -24,7 +60,7 @@ function pushDrawerRoute(screenName, params, navigationRef) { | |
|
||
if (activeReportID === params.reportID) { | ||
if (state.type !== 'drawer') { | ||
navigationRef.current.dispatch(StackActions.pop()); | ||
navigateBackToRootDrawer(navigationRef); | ||
} | ||
return DrawerActions.closeDrawer(); | ||
} | ||
|
@@ -52,4 +88,5 @@ function pushDrawerRoute(screenName, params, navigationRef) { | |
|
||
export default { | ||
pushDrawerRoute, | ||
navigateBackToDrawer: navigateBackToRootDrawer, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @marcaaron Oh, my editor messed it up. Should I send another PR now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Follow up #5937 |
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -137,39 +137,10 @@ function dismissModal(shouldOpenDrawer = false) { | |
? shouldOpenDrawer | ||
: false; | ||
|
||
let isLeavingDrawerNavigator; | ||
|
||
// This should take us to the first view of the modal's stack navigator | ||
navigationRef.current.dispatch((state) => { | ||
// If this is a nested drawer navigator then we pop the screen and | ||
// prevent calling goBack() as it's default behavior is to toggle open the active drawer | ||
if (state.type === 'drawer') { | ||
isLeavingDrawerNavigator = true; | ||
return StackActions.pop(); | ||
} | ||
|
||
// If there are multiple routes then we can pop back to the first route | ||
if (state.routes.length > 1) { | ||
return StackActions.popToTop(); | ||
} | ||
|
||
// Otherwise, we are already on the last page of a modal so just do nothing here as goBack() will navigate us | ||
// back to the screen we were on before we opened the modal. | ||
return StackActions.pop(0); | ||
}); | ||
|
||
if (isLeavingDrawerNavigator) { | ||
return; | ||
CustomActions.navigateBackToDrawer(navigationRef); | ||
if (normalizedShouldOpenDrawer) { | ||
openDrawer(); | ||
} | ||
|
||
// Navigate back to where we were before we launched the modal | ||
goBack(shouldOpenDrawer); | ||
|
||
if (!normalizedShouldOpenDrawer) { | ||
return; | ||
} | ||
|
||
openDrawer(); | ||
Comment on lines
-165
to
-172
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @marcaaron I didn't understand this part. Why were this code opening Drawer two times?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you rephrase this question? I'm not really sure what you are asking. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked at the code. It seems to me that this piece of code was calling
Or am I wrong? |
||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,7 +83,6 @@ class EnableStep extends React.Component { | |
title: translate('workspace.bankAccount.chatWithConcierge'), | ||
icon: ChatBubble, | ||
onPress: () => { | ||
Navigation.dismissModal(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comming from #34456, remove dismiss modal here potential causes that bug. |
||
navigateToConciergeChat(); | ||
}, | ||
shouldShowRightIcon: true, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still have nested drawer navigators in use? I think we got rid of the one case. In any case, I think if we want to keep this logic we should make a distinction between a "nested drawer navigator" and a "main" or "root" drawer navigator.
Otherwise, these variable names are confusing. I understand this code but someone might have a thought like, "Why do we have
isLeavingDrawerNavigator
in a method callednavigateBackToDrawer()
? Shouldn'tisLeavingDrawerNavigator
always befalse
?" Maybe it should beisLeavingNestedDrawerNavigator
instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree.