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

Navigate to correct report on push notification tap #8353

Merged
merged 6 commits into from
Apr 5, 2022
+25 −11
Merged

Navigate to correct report on push notification tap #8353

Show file tree
Hide file tree
Changes from 1 commit
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
21 changes: 21 additions & 0 deletions src/libs/Navigation/Navigation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@ function setDidTapNotification() {
didTapNotificationBeforeReady = true;
}

/**
* Returns true if the Navigation is ready to navigate
* @returns {boolean}
AndrewGable marked this conversation as resolved.
Show resolved Hide resolved
*/
function isReady() {
return navigationRef.isReady();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

noting: we have a usage of navigationRef.isReady() in the canNavigate() method. Looks like they're kind of doing the same thing - but one is logging if we aren't ready. Is that something we should use instead of creating a new method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 - I can use canNavigate instead


/**
* @param {String} methodName
* @param {Object} params
Expand Down Expand Up @@ -185,6 +193,17 @@ function isActiveRoute(routePath) {
return path === routePath;
}

/**
* Returns whether we have a chat visible or not
* @returns {boolean}
*/
function isChatVisible() {
const path = navigationRef.current && navigationRef.current.getCurrentRoute().name
? getPathFromState(navigationRef.current.getState(), linkingConfig.config).slice(0, 3)
: '';
return path === '/r/';
}
AndrewGable marked this conversation as resolved.
Show resolved Hide resolved

/**
* Alternative to the `Navigation.dismissModal()` function that we can use inside
* the render function of other components to avoid breaking React rules about side-effects.
Expand Down Expand Up @@ -217,6 +236,8 @@ export default {
navigate,
dismissModal,
isActiveRoute,
isChatVisible,
isReady,
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.isReady()) {
// If a chat is visible other than the one we are trying to navigate to, then we need to navigate back
if (Navigation.isChatVisible() && Navigation.isActiveRoute(`r/${reportID}`)) {
AndrewGable 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