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

fix: add proper fallback route for private notes pages #28273

Merged
merged 6 commits into from
Oct 2, 2023
Merged

fix: add proper fallback route for private notes pages #28273

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
501e003
fix: add proper fallback routes for private notes pages
BhuvaneshPatil Sep 26, 2023
ba36c13
revert changes for list page
BhuvaneshPatil Sep 27, 2023
78a6926
nit: remove lodash usage and get property values directly
BhuvaneshPatil Sep 28, 2023
b5a7437
add fallback route to view page
BhuvaneshPatil Sep 29, 2023
0a36072
Merge branch 'main' of https://github.com/Expensify/App into 28011-ad…
BhuvaneshPatil Sep 29, 2023
7077466
Merge branch 'main' of https://github.com/Expensify/App into 28011-ad…
BhuvaneshPatil Oct 2, 2023
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
3 changes: 2 additions & 1 deletion src/pages/PrivateNotes/PrivateNotesEditPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ function PrivateNotesEditPage({route, personalDetailsList, session, report}) {
Keyboard.dismiss();

// Take user back to the PrivateNotesView page
Navigation.goBack(ROUTES.HOME);
Navigation.goBack(ROUTES.PRIVATE_NOTES_VIEW.getRoute(report.reportID, route.params.accountID));
};

return (
Expand All @@ -106,6 +106,7 @@ function PrivateNotesEditPage({route, personalDetailsList, session, report}) {
<HeaderWithBackButton
title={translate('privateNotes.title')}
subtitle={translate('privateNotes.myNote')}
onBackButtonPress={() => Navigation.goBack(ROUTES.PRIVATE_NOTES_VIEW.getRoute(report.repotID, route.params.accountID))}
Copy link
Contributor

Choose a reason for hiding this comment

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

repotID lol 😅
we're fixing it here

shouldShowBackButton
onCloseButtonPress={() => Navigation.dismissModal()}
/>
Expand Down
1 change: 1 addition & 0 deletions src/pages/PrivateNotes/PrivateNotesViewPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ function PrivateNotesViewPage({route, personalDetailsList, session, report}) {
>
<HeaderWithBackButton
title={translate('privateNotes.title')}
onBackButtonPress={() => Navigation.goBack(ROUTES.PRIVATE_NOTES_LIST.getRoute(report.reportID))}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is tricky because we will conditionally show the list page. I was working on this here #28105 but couldn't prioritize it.

In case a user has a single note that they can access we will take them right away to the view page. So you can't always be sure that view page would go back to list page.

Navigaton flow:
Single note: Private notes button -> view page -> edit page
Multiple notes: Private notes button -> list page -> view page -> edit page

Copy link
Contributor

Choose a reason for hiding this comment

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

Navigating them back gets even trickier because you need to know which page they came from(it could be the profile page or the report details page). That's why I sort of just relied on the goBack default behaviour to sort that out from the navigation stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, great catch! So I think it's okay to revert this change. Just like there are also multiple flows for going back from the notes list page to the report detail page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, so we shall only change the PrivateNotesEditPage as we are sure that user would have came from PrivateNotesViewPage, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

No no, have a look at the navigation flow I have mentioned here #28273 (comment)

Navigaton flow:
Single note: Private notes button -> view page -> edit page
Multiple notes: Private notes button -> list page -> view page -> edit page

So it is between the private notes button and the view page.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you misunderstood me here #28273 (comment)

Aaah, yes sorry, I went with negation. Since you are fixing the navigation behaviour here I would say we also want to fix for the View page, can you look into it to see if we have a way?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am also sort of confused, why we don't retain the navigation stack when user refreshes the page. Does that happen with all the pages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have seen same behaviour for workspace pages that we have added fallback route.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@techievivek Can you tell me scenarios where I can see multiple private notes. Because currently I can only see one in workspace as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@techievivek I would like to use logic that you have used https://github.com/Expensify/App/pull/28105/files#diff-65c096044d5f69b35bcdec14c99c4fda4580759df9b1a7c36650d58eea276f1dR3569 here.

To check number of reports and based on that navigate. Let me know if this sounds right to you

subtitle={isCurrentUserNote ? translate('privateNotes.myNote') : `${lodashGet(personalDetailsList, [route.params.accountID, 'login'], '')} note`}
shouldShowBackButton
onCloseButtonPress={() => Navigation.dismissModal()}
Expand Down