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 all 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 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();
}
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 (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
Expand All @@ -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();
}
Expand Down Expand Up @@ -52,4 +88,5 @@ function pushDrawerRoute(screenName, params, navigationRef) {

export default {
pushDrawerRoute,
navigateBackToDrawer: navigateBackToRootDrawer,
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.

@marcaaron Oh, my editor messed it up. Should I send another PR now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Follow up #5937

};
35 changes: 3 additions & 32 deletions src/libs/Navigation/Navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
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
1 change: 0 additions & 1 deletion src/pages/ReimbursementAccount/EnableStep.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ class EnableStep extends React.Component {
title: translate('workspace.bankAccount.chatWithConcierge'),
icon: ChatBubble,
onPress: () => {
Navigation.dismissModal();
Copy link
Contributor

Choose a reason for hiding this comment

The 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,
Expand Down
10 changes: 2 additions & 8 deletions src/pages/ReimbursementAccount/ValidationStep.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ class ValidationStep extends React.Component {
super(props);

this.submit = this.submit.bind(this);
this.navigateToConcierge = this.navigateToConcierge.bind(this);

this.state = {
amount1: ReimbursementAccountUtils.getDefaultStateForField(props, 'amount1', ''),
Expand Down Expand Up @@ -150,11 +149,6 @@ class ValidationStep extends React.Component {
return value;
}

navigateToConcierge() {
Navigation.dismissModal();
navigateToConciergeChat();
}

render() {
const state = lodashGet(this.props, 'reimbursementAccount.achData.state');

Expand Down Expand Up @@ -182,7 +176,7 @@ class ValidationStep extends React.Component {
{' '}
{this.props.translate('common.please')}
{' '}
<TextLink onPress={this.navigateToConcierge}>
<TextLink onPress={navigateToConciergeChat}>
{this.props.translate('common.contactUs')}
</TextLink>
.
Expand Down Expand Up @@ -237,7 +231,7 @@ class ValidationStep extends React.Component {
menuItems={[{
title: this.props.translate('validationStep.letsChatCTA'),
icon: ChatBubble,
onPress: this.navigateToConcierge,
onPress: navigateToConciergeChat,
shouldShowRightIcon: true,
}]}
>
Expand Down
29 changes: 4 additions & 25 deletions src/pages/settings/AboutPage.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import React from 'react';
import PropTypes from 'prop-types';
import {View, ScrollView} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import HeaderWithCloseButton from '../../components/HeaderWithCloseButton';
import Navigation from '../../libs/Navigation/Navigation';
import ROUTES from '../../ROUTES';
Expand All @@ -17,27 +15,17 @@ import {
} from '../../components/Icon/Expensicons';
import ScreenWrapper from '../../components/ScreenWrapper';
import withLocalize, {withLocalizePropTypes} from '../../components/withLocalize';
import compose from '../../libs/compose';
import MenuItem from '../../components/MenuItem';
import Logo from '../../../assets/images/new-expensify.svg';
import {version} from '../../../package.json';
import {fetchOrCreateChatReport} from '../../libs/actions/Report';
import ONYXKEYS from '../../ONYXKEYS';
import {navigateToConciergeChat} from '../../libs/actions/Report';
import {openExternalLink} from '../../libs/actions/Link';

const propTypes = {
/** Onyx Props */

/** Session info for the currently logged in user. */
session: PropTypes.shape({
/** Currently logged in user email */
email: PropTypes.string,
}).isRequired,

...withLocalizePropTypes,
};

const AboutPage = ({translate, session}) => {
const AboutPage = ({translate}) => {
const menuItems = [
{
translationKey: 'initialSettingsPage.aboutPage.appDownloadLinks',
Expand Down Expand Up @@ -65,9 +53,7 @@ const AboutPage = ({translate, session}) => {
{
translationKey: 'initialSettingsPage.aboutPage.reportABug',
icon: Bug,
action: () => {
fetchOrCreateChatReport([session.email, CONST.EMAIL.CONCIERGE], true);
},
action: navigateToConciergeChat,
},
];

Expand Down Expand Up @@ -159,11 +145,4 @@ const AboutPage = ({translate, session}) => {
AboutPage.propTypes = propTypes;
AboutPage.displayName = 'AboutPage';

export default compose(
withLocalize,
withOnyx({
session: {
key: () => ONYXKEYS.SESSION,
},
}),
)(AboutPage);
export default withLocalize(AboutPage);
2 changes: 0 additions & 2 deletions src/pages/workspace/travel/WorkspaceTravelVBAView.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {RocketOrange} from '../../../components/Icon/Illustrations';
import WorkspaceSection from '../WorkspaceSection';
import {openExternalLink, openOldDotLink} from '../../../libs/actions/Link';
import {navigateToConciergeChat} from '../../../libs/actions/Report';
import Navigation from '../../../libs/Navigation/Navigation';

const propTypes = {
...withLocalizePropTypes,
Expand All @@ -34,7 +33,6 @@ const WorkspaceTravelVBAView = ({translate}) => (
{
title: translate('workspace.travel.bookTravelWithConcierge'),
onPress: () => {
Navigation.dismissModal();
navigateToConciergeChat();
},
icon: Concierge,
Expand Down