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.

Sign up for GitHub

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

Jump to bottom

Navigate to correct report on push notification tap #8353

Merged
merged 6 commits into from
Apr 5, 2022
Merged

Navigate to correct report on push notification tap #8353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
904e3ee
Navigate to correct report on push notification tap
AndrewGable Mar 30, 2022
7b3c158
Tweak checking for report
AndrewGable Mar 31, 2022
0763eab
Update comment
AndrewGable Mar 31, 2022
3467f88
Update src/libs/Navigation/Navigation.js
AndrewGable Apr 4, 2022
86050cc
Use `getActiveRoute` in `isActiveRoute`
AndrewGable Apr 4, 2022
2ef1277
Use `canNavigate` instead of `isReady`
AndrewGable Apr 4, 2022
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
23 changes: 14 additions & 9 deletions src/libs/Navigation/Navigation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
import _ from 'underscore';
import React from 'react';
import {Keyboard} from 'react-native';
import {
StackActions,
DrawerActions,
getPathFromState,
} from '@react-navigation/native';
import {DrawerActions, getPathFromState, StackActions} from '@react-navigation/native';
import PropTypes from 'prop-types';
import Onyx from 'react-native-onyx';
import Log from '../Log';
Expand Down Expand Up @@ -168,6 +164,16 @@ function dismissModal(shouldOpenDrawer = false) {
}
}

/**
* Returns the current active route
* @returns {String}
*/
function getActiveRoute() {
return navigationRef.current && navigationRef.current.getCurrentRoute().name
? getPathFromState(navigationRef.current.getState(), linkingConfig.config)
: '';
}

/**
* Check whether the passed route is currently Active or not.
*
Expand All @@ -179,10 +185,7 @@ function dismissModal(shouldOpenDrawer = false) {
*/
function isActiveRoute(routePath) {
// We remove First forward slash from the URL before matching
const path = navigationRef.current && navigationRef.current.getCurrentRoute().name
? getPathFromState(navigationRef.current.getState(), linkingConfig.config).substring(1)
: '';
return path === routePath;
return getActiveRoute().substring(1) === routePath;
}
AndrewGable marked this conversation as resolved.
Show resolved Hide resolved

/**
Expand Down Expand Up @@ -214,9 +217,11 @@ DismissModal.defaultProps = {
};

export default {
canNavigate,
navigate,
dismissModal,
isActiveRoute,
getActiveRoute,
goBack,
DismissModal,
closeDrawer,
Expand Down
13 changes: 11 additions & 2 deletions src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -797,8 +797,17 @@ function subscribeToReportCommentPushNotifications() {

// Open correct report when push notification is clicked
PushNotification.onSelected(PushNotification.TYPE.REPORT_COMMENT, ({reportID}) => {
Navigation.setDidTapNotification();
Linking.openURL(`${CONST.DEEPLINK_BASE_URL}${ROUTES.getReportRoute(reportID)}`);
if (Navigation.canNavigate('navigate')) {
// If a chat is visible other than the one we are trying to navigate to, then we need to navigate back
if (Navigation.getActiveRoute().slice(1, 2) === ROUTES.REPORT && !Navigation.isActiveRoute(`r/${reportID}`)) {
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
Navigation.goBack();
}
Navigation.navigate(ROUTES.getReportRoute(reportID));
} else {
// Navigation container is not yet ready, use deeplinking to open to correct report instead
Navigation.setDidTapNotification();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to remember why we have setDidTapNotification(), but can't quite recall. Is it not something we need in the first part of this if/else and only when we are using deep linking? Maybe we can leave a comment if it's not too hard to figure out.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC the reason we have setDidTapNotification is:

// Navigation container is not yet ready, use deeplinking to open correct report instead.
// By default, the main drawer will start in the open state when the navigation container mounts.
// So you'll see the LHN, but we instead want to link to a specific report.
// We use the `setDidTapNotification` flag here to force the main drawer to mount in the closed state.

Linking.openURL(`${CONST.DEEPLINK_BASE_URL}${ROUTES.getReportRoute(reportID)}`);
}
});
}

Expand Down