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.

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

Merged
merged 7 commits into from
Oct 18, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 38 additions & 1 deletion src/libs/Navigation/CustomActions.js
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 navigateBackToDrawer(navigationRef) {
Copy link
Contributor

@marcaaron marcaaron Oct 18, 2021

Choose a reason for hiding this comment

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

I'm having trouble with this name because it implies we are calling this when the drawer is not active. It looks like something I would call if I was not on the drawer to take me back to the drawer. However, if you look we also have logic for handling when a nested drawer is active.

Ultimately, I find handling both responsibilities confusing and would love to figure out an alternative.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't find any good name for this method. To me, it seems to do multiple things. But I just removed the duplicate behaviour from our navigation and put it into one method. I found it difficult to break this into two and still abstract away the duplicates.

I will try to find something cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok no worries. I think with the better naming this is acceptable now.

let isLeavingDrawerNavigator = 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') {
isLeavingDrawerNavigator = true;
return StackActions.pop();
}
Copy link
Contributor

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 called navigateBackToDrawer()? Shouldn't isLeavingDrawerNavigator always be false?" Maybe it should be isLeavingNestedDrawerNavigator instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I agree.


// 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;
}

// 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
Expand All @@ -24,7 +60,7 @@ function pushDrawerRoute(screenName, params, navigationRef) {

if (activeReportID === params.reportID) {
if (state.type !== 'drawer') {
navigationRef.current.dispatch(StackActions.pop());
navigateBackToDrawer(navigationRef);
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing that sticks out to me is that we are checking state.type !== 'drawer' and then again checking in navigateBackToDrawer(). So it seems the method has more responsibility than it needs.

Copy link
Member Author

@parasharrajat parasharrajat Oct 18, 2021

Choose a reason for hiding this comment

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

Yeah. I think its more cleaner if we change the name to navigateBackToRootDrawer or navigateBackToReportDrawer.

}
return DrawerActions.closeDrawer();
}
Expand Down Expand Up @@ -52,4 +88,5 @@ function pushDrawerRoute(screenName, params, navigationRef) {

export default {
pushDrawerRoute,
navigateBackToDrawer,
};
35 changes: 3 additions & 32 deletions src/libs/Navigation/Navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,39 +114,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
Copy link
Member Author

Choose a reason for hiding this comment

The 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?

  1. via goback when shouldOpenDrawer is true.
  2. Then OpenDrawer call which happens when shouldOpenDrawer is true.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

@parasharrajat parasharrajat Sep 14, 2021

Choose a reason for hiding this comment

The 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 openDrawer two times.

  1. Via goback when shouldOpenDrawer is true.
  2. Then OpenDrawer call which happens when shouldOpenDrawer is true.

Or am I wrong?

}

/**
Expand Down