ERA-8169: Confirmation modal when navigating away from unsaved changes #831

luixlive · 2023-01-13T18:39:42Z

Ticket: https://allenai.atlassian.net/browse/ERA-8169
Figma: https://www.figma.com/file/1u3VbK9kbOEuUg9Wi8AHRW/Patrol-%26-Report-UI-Refresh-FINAL?node-id=3141%3A29671&t=YcmsZAmHiNIzQQZH-0
Env: https://era-8169.pamdas.org/

Description
So, this is a tricky one. This PR introduces the capability to "block" navigation attempts through useNavigate hook and Link component in order to show prompt messages when the user tries to leave, for example, an incomplete (dirty) form. This functionality used to be included in react-router (search for <Prompt> or useBlocker in v5). Those functionalities are supposedly a WIP in v6, but there's no specified release date (refer to remix-run/react-router#8139).

This PR then includes a new hook useBlockNavigation (I'm open to name suggestions) that receives a single boolean parameter to block / unblock the navigation. For example we may want to block it if a form is modified, but keep it unblocked if it's not. And it returns an object with three properties:

isNavigationAttemptPending: boolean set as true if the user tried to leave the page. While the variable is true it means that the app is in a pending navigation state, until we call one of the following methods.
cancelNavigationAttempt: method to tell the navigation logic that the pending navigation attempt can be ignored
continueNavigationAttempt: method to tell the navigation logic that the pending navigation attempt should be addressed (thus redirecting the user to the desired page)

This logic is actually not invented by me, but what I found to be the common pattern to handle navigation blockers in React. Just implemented by me 🤠 The navigation blocking, navigation attempt and navigation attempt resolution logic is abstracted in our NavigationContextProvider and our custom useNavigate and the new Link components.

The PR also includes a new NavigationPromptModal that receives a single required prop when and a few customization optional props to change the modal title, description and buttons content. It uses the useBlockNavigation hook internally to block the navigation when the prop when is true. Whenever there is a pending attempt to navigate, the prompt appears asking the user to choose between canceling or continuing the attempt. This component makes it really easy to reuse all the navigation blocking logic showing a prompt modal as it's intended in the report and patrol new UI.

… message in report detail view

luixlive · 2023-01-16T22:15:15Z

src/Link/index.js

+
+import { NavigationContext } from '../NavigationContextProvider';
+
+const Link = ({ onClick, ...rest }) => {


Custom Link wrapper that "adds" our blocking functionality on top of the normal Link

luixlive · 2023-01-16T22:18:31Z

src/NavigationContextProvider/index.js


 export const NavigationContext = createContext();

 const NavigationContextProvider = ({ children }) => {
+  const [isNavigationAttemptPending, setIsNavigationAttemptPending] = useState(false);
+  const [isNavigationBlocked, setIsNavigationBlocked] = useState(false);
+  const [navigationAttemptResolution, setNavigationAttemptResolution] = useState(null);


isNavigationAttemptPending: true when the navigation is blocked and the user tried to leave the page, so the app is waiting for a resolution on that attempt.
isNavigationBlocked: means the navigation is currently blocked in the app, intercepting every Link and useNavigate navigation attempts
navigationAttemptResolution: null while there's no resolution. It can only have a non-null value when there is a navigation attempt pending and the user chose to either continue with it (sets this flag as true) or cancel it (sets it to false).

The phrasing here is pretty confusing. For example: a "resolution" is usually a noun describing the way something was resolved. I would expect a state container holding a resolution to contain what happens on resolution, not a boolean flag for if something is resolved.

Likewise, the verbiage around blocked, pending, attempted, etc could be simplified

luixlive · 2023-01-16T22:19:16Z

src/NavigationPromptModal/index.js

+
+import styles from './styles.module.scss';
+
+const NavigationPromptModal = ({


New prompt component that automatically blocks the navigation (if when prop is true) and shows a modal asking for user interaction to continue with the redirection or cancel it.

luixlive · 2023-01-16T22:20:00Z

src/hooks/useBlockNavigation/index.js

+
+import { NavigationContext } from '../../NavigationContextProvider';
+
+const useBlockNavigation = (when) => {


New hook to block navigation if when is true and return the necessary properties to handle a navigation attempt.

Alcoto95

I just noticed the following:

I think there should be no confirmation for this case

Screen.Recording.2023-01-17.at.11.27.10.mov

luixlive · 2023-01-18T21:44:54Z

@Alcoto95 we should have that covered now 😎

Alcoto95 · 2023-01-18T22:24:52Z

I noticed a couple more things with the latest commit:

I see that it no longer identifies the state of the details, so if I do the following it keeps on asking for confirmation even when I came back to the state it was before editing:

Screen.Recording.2023-01-18.at.16.15.04.mov

The other one is the following:

Screen.Recording.2023-01-18.at.16.17.36.mov

I can't create an incident by adding an additional event without updating anything from that second event in its details. We should be able to add the event without updating any details.

… reports

Alcoto95

Thanks for the fixes! LGTM now 🎉 don't forget to turn off the FF

JoshuaVulcan

My comments are incomplete, but reading through the code raised a suspicion which was confirmed by some UI testing...this doesn't intercept/prevent browser-triggered navigation, just direct interaction with elements. We definitely need to support that.

JoshuaVulcan · 2023-01-20T02:34:59Z

src/NavigationContextProvider/index.js


 export const NavigationContext = createContext();

 const NavigationContextProvider = ({ children }) => {
+  const [isNavigationAttemptPending, setIsNavigationAttemptPending] = useState(false);
+  const [isNavigationBlocked, setIsNavigationBlocked] = useState(false);
+  const [navigationAttemptResolution, setNavigationAttemptResolution] = useState(null);


The phrasing here is pretty confusing. For example: a "resolution" is usually a noun describing the way something was resolved. I would expect a state container holding a resolution to contain what happens on resolution, not a boolean flag for if something is resolved.

Likewise, the verbiage around blocked, pending, attempted, etc could be simplified

JoshuaVulcan · 2023-01-20T02:35:45Z

src/Link/index.js

+
+  const linkRef = useRef();
+
+  const [localNavigationAttemptBlocked, setLocalNavigationAttemptBlocked] = useState(false);


The pattern of having a context-inherited value and also a locally-scoped identical value feels wonky to me

Yeah... It is because we can have multiple link components rendered in the page at the same time and all of them would try to autoclick itself every time a blocked navigation attempt is "resolved". So I had to somehow keep a internal state that tells if this link is the one that the user tried to click.

JoshuaVulcan · 2023-01-20T02:36:22Z

src/Link/index.js

+  const linkRef = useRef();
+
+  const [localNavigationAttemptBlocked, setLocalNavigationAttemptBlocked] = useState(false);
+  const [localNavigationUnblocked, setLocalNavigationUnblocked] = useState(false);


Wouldn't you want to initialize these state values with the initial values coming from useContext(NavigationContext)?

Hmm 🤔 yeah, I think you're right

JoshuaVulcan · 2023-01-20T02:36:46Z

src/Link/index.js

+      event.preventDefault();
+
+      setLocalNavigationAttemptBlocked(true);
+      navigationAttemptBlocked();


This is an event handler with no on prefix, which is probably part of why it reads so strangely when trying to parse the code

luixlive · 2023-01-26T23:29:59Z

@JoshuaVulcan about the browser navigation, I couldn't find a way to make it work with Tiffany's designs. We have the event onBeforeUnload that will show a prompt, but the message or the style can't be modified: https://stackoverflow.com/questions/276660/how-can-i-override-the-onbeforeunload-dialog-and-replace-it-with-my-own I can add it as part of this same logic. I think it would make sense to add it in the navigation context component since that's what connects everything. But we should be aware that we will show different prompts depending on if the navigation happens in our code or at the browser level.

…ompt for browser events

luixlive · 2023-01-30T22:17:22Z

@JoshuaVulcan I tried to implement unstable_useBlocker as it was recently released, but with no success since we need to change our BrowserRouter to a data router. I guess we can wait a little bit more until that functionality is fully implemented and stable. For now, I changed the phrasing following their same pattern of having a blocker object with properties: state (unblocked | blocked | proceeding), reset and proceed. I think that simplifies the flow a little, while maintaining consistency with the react router implementation.
I also added the event listener for beforeunload to show a browser prompt in browser navigation events, but as I mentioned before, we can't change the styles for that one. Also, I noticed that it doesn't work for the left arrow of Google Chrome (back button), I wonder if that is delegated in Javascript and the react router takes control of it 🤔 But it works nice if we want to close the window.

JoshuaVulcan

Looking quite good! A few questions and comments, mostly ready to fly 🚀 🚀 🚀

JoshuaVulcan · 2023-01-31T05:59:27Z

src/Link/index.js

+      setSkipBlocker(true);
+      setWasClicked(false);
+
+      setTimeout(() => linkRef.current.click());


Do we lose anything by not forwarding the original blocked click event through to the handler?

Hmm that's a good question, didn't thought about it. I couldn't think of anything, but it should be pretty easy to store the event on a ref or something. Just a question, Idk how to forward an event, is it as easy as:

setTimeout(() => linkRef.current.click(blockedEventRef.current));

??

JoshuaVulcan · 2023-01-31T06:03:23Z

src/NavigationContextProvider/index.test.js

+  });
+
+  test('unblocks the navigation', async () => {
+    const ChildComponent = () => {


I'm seeing some funky test patterns here. For example, in unblocks the navigation, you're blocking and then immediately unblocking navigation without verifying the "blocked" state in the lifespan of the test (yes it's tested separately in the previous test, but that's different); this means if it ever fails in this test, you wouldn't know.

Since you're only really testing output values from useContext in a lot of these tests, why not use renderHook with the provider as the wrapper?
Like this: https://react-hooks-testing-library.com/usage/advanced-hooks#context
Then you could directly verify the outcomes as returned values, rather than rendering placeholder components and reading the stringified output.

Taking a look at that 👀

JoshuaVulcan · 2023-01-31T06:08:55Z

src/utils/objects.js

@@ -4,7 +4,15 @@ import isEqual from 'react-fast-compare';

 export const extractObjectDifference = (object, original) => transform(object, (result, value, key) => {
  if (!isEqual(value, original[key])) {
-    result[key] = isPlainObject(value) && isPlainObject(original[key]) ? extractObjectDifference(value, original[key]) : value;


Can you elaborate on these changes? It's a core utility, it would be good to know about possible side effects in other portions of the code.

Yes! It is good that you mention this. So you can see that this portion:

result[key] = isPlainObject(value) && isPlainObject(original[key]) ? extractObjectDifference(value, original[key]) : value;

is logically exactly the same than:

if (isPlainObject(value) && isPlainObject(original[key])) { result[key] = extractObjectDifference(value, original[key]); } else { result[key] = value; }

Just some syntax change. But, in the first branch of the condition I added a second validation:

if (Object.keys(objectDifference).length > 0) { result[key] = objectDifference; }

Basically, if the extractObjectDifference function returns an empty object, I don't add it to the result.

The reasoning behind is that sometimes we got something like this when extracting the difference from the original and the edited report:
{ someProperty: {} }
Which basically means that there is no really a difference between the two, just that it found some empty property in one that is not present in the second one. Technically, there is no difference between the forms, but for some reason at some point we added an empty object as property in one of them. It happened in a specific scenario in collections, I can't remember the name of the property, I could easily find it if I remove these changes. But the issue was that the save button was enabled and the confirmation modal was showing up even if the user didn't change anything in the report collection. We depend on the output of this function to enable those two things.

…avigation context tests

luixlive · 2023-01-31T20:30:48Z

@JoshuaVulcan ready for another pass. I'd just like to know your thoughts about my comment in the utility function change

JoshuaVulcan · 2023-02-02T18:36:31Z

@luixlive ready for FF toggle and merge

…rop to NavigationPrompt to force its visibility

luixlive · 2023-02-03T17:33:21Z

@JoshuaVulcan @Alcoto95 my last commit fixes the case of the second added report. It does it by:

Enabling us to force the showing of the navigation prompt (aka confirmation modal) with the show prop. So now we can display it even if there was no navigation attempt (like the added report scenario).
This brought a new issue, having two NavigationPrompt s rendered at the same time, implies having two useNavigationBlocker hooks at once. Our NavigationContext logic to block didn't support multiple blockers. As soon as one of them unblocked the navigation, the other blocker was being ignored. So now instead of a boolean flag isNavigationBlokcer we have an array of blocker ids. As long as there is at least one id in there, the navigation is blocked. That explains the new useRef(uuid()) in the hook.

JoshuaVulcan · 2023-02-07T20:09:19Z

@luixlive LGTM!

ERA-8169: Add navigation blocking functionality plus the confirmation…

f09d3b1

… message in report detail view

luixlive marked this pull request as draft January 13, 2023 18:39

ERA-8169: Add unit tests

a006fd7

luixlive marked this pull request as ready for review January 16, 2023 22:13

luixlive requested review from JoshuaVulcan, Alcoto95 and gaboDev January 16, 2023 22:13

luixlive commented Jan 16, 2023

View reviewed changes

luixlive added 2 commits January 16, 2023 16:26

Merge branch 'develop' into ERA-8169

2f04614

Merge branch 'develop' into ERA-8169

08e54c1

Alcoto95 suggested changes Jan 17, 2023

View reviewed changes

ERA-8169: Fix incidents showing prompt without changes

55195cd

luixlive requested a review from Alcoto95 January 18, 2023 21:44

ERA-8169: Fix empty fields changes detection and save button in added…

638130d

… reports

Alcoto95 approved these changes Jan 19, 2023

View reviewed changes

JoshuaVulcan requested changes Jan 26, 2023

View reviewed changes

ERA-8169: Simplify phrasing over blocking navigation logic and add pr…

a808fc1

…ompt for browser events

JoshuaVulcan requested changes Jan 31, 2023

View reviewed changes

ERA-8169: Send original blocked event in Link and use renderHook in N…

b29c891

…avigation context tests

luixlive requested a review from JoshuaVulcan January 31, 2023 20:29

JoshuaVulcan approved these changes Feb 2, 2023

View reviewed changes

luixlive added 2 commits February 2, 2023 14:49

ERA-8169: Protect blocker on added report

69a0324

ERA-8169: Support multiple navigation blockers at once and add show p…

c89410a

…rop to NavigationPrompt to force its visibility

luixlive merged commit 4afbe94 into develop Feb 7, 2023

luixlive deleted the ERA-8169 branch February 7, 2023 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ERA-8169: Confirmation modal when navigating away from unsaved changes #831

ERA-8169: Confirmation modal when navigating away from unsaved changes #831

luixlive commented Jan 13, 2023 •

edited

luixlive Jan 16, 2023

luixlive Jan 16, 2023

JoshuaVulcan Jan 20, 2023

luixlive Jan 16, 2023

luixlive Jan 16, 2023

Alcoto95 left a comment

luixlive commented Jan 18, 2023

Alcoto95 commented Jan 18, 2023

Alcoto95 left a comment

JoshuaVulcan left a comment

JoshuaVulcan Jan 20, 2023

JoshuaVulcan Jan 20, 2023

luixlive Jan 26, 2023

JoshuaVulcan Jan 20, 2023

luixlive Jan 26, 2023

JoshuaVulcan Jan 20, 2023

luixlive commented Jan 26, 2023

luixlive commented Jan 30, 2023

JoshuaVulcan left a comment

JoshuaVulcan Jan 31, 2023

luixlive Jan 31, 2023

JoshuaVulcan Jan 31, 2023

luixlive Jan 31, 2023

JoshuaVulcan Jan 31, 2023

luixlive Jan 31, 2023

luixlive commented Jan 31, 2023

JoshuaVulcan commented Feb 2, 2023

luixlive commented Feb 3, 2023

JoshuaVulcan commented Feb 7, 2023


		import { NavigationContext } from '../NavigationContextProvider';

		const Link = ({ onClick, ...rest }) => {


		import styles from './styles.module.scss';

		const NavigationPromptModal = ({


		import { NavigationContext } from '../../NavigationContextProvider';

		const useBlockNavigation = (when) => {


		const linkRef = useRef();

		const [localNavigationAttemptBlocked, setLocalNavigationAttemptBlocked] = useState(false);

ERA-8169: Confirmation modal when navigating away from unsaved changes #831

ERA-8169: Confirmation modal when navigating away from unsaved changes #831

Conversation

luixlive commented Jan 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alcoto95 left a comment

Choose a reason for hiding this comment

luixlive commented Jan 18, 2023

Alcoto95 commented Jan 18, 2023

Alcoto95 left a comment

Choose a reason for hiding this comment

JoshuaVulcan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luixlive commented Jan 26, 2023

luixlive commented Jan 30, 2023

JoshuaVulcan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luixlive commented Jan 31, 2023

JoshuaVulcan commented Feb 2, 2023

luixlive commented Feb 3, 2023

JoshuaVulcan commented Feb 7, 2023

luixlive commented Jan 13, 2023 •

edited