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

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

Merged
merged 10 commits into from
Feb 7, 2023
2 changes: 1 addition & 1 deletion .env
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@ REACT_APP_BASE_MAP_STYLES='mapbox://styles/vjoelm/ciobuir0n0061bdnj1c54oakh?opti

# Feature flags
REACT_APP_ENABLE_PATROL_NEW_UI=false
REACT_APP_ENABLE_REPORT_NEW_UI=false
REACT_APP_ENABLE_REPORT_NEW_UI=true
REACT_APP_ENABLE_GEOPERMISSION_UI=true
REACT_APP_ENABLE_EVENT_GEOMETRY=true
46 changes: 46 additions & 0 deletions src/Link/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import React, { useCallback, useContext, useEffect, useRef, useState } from 'react';
import { Link as RouterLink } from 'react-router-dom';

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

const Link = ({ onClick, ...rest }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

const {
isNavigationBlocked,
navigationAttemptBlocked,
navigationAttemptResolution,
navigationAttemptUnblocked,
} = useContext(NavigationContext);

const linkRef = useRef();

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const [localNavigationUnblocked, setLocalNavigationUnblocked] = useState(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm 🤔 yeah, I think you're right


const handleClick = useCallback((event) => {
if (!localNavigationUnblocked && isNavigationBlocked) {
event.preventDefault();

setLocalNavigationAttemptBlocked(true);
navigationAttemptBlocked();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

} else {
onClick?.(event);
setLocalNavigationUnblocked(false);
}
}, [isNavigationBlocked, localNavigationUnblocked, navigationAttemptBlocked, onClick]);

useEffect(() => {
if (localNavigationAttemptBlocked && navigationAttemptResolution !== null) {
if (navigationAttemptResolution) {
setLocalNavigationUnblocked(true);
setTimeout(() => linkRef.current.click());
}

setLocalNavigationAttemptBlocked(false);
navigationAttemptUnblocked();
}
}, [localNavigationAttemptBlocked, navigationAttemptResolution, navigationAttemptUnblocked]);

return <RouterLink onClick={handleClick} ref={linkRef} {...rest} />;
};

export default Link;
75 changes: 75 additions & 0 deletions src/Link/index.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import React from 'react';
import { useLocation } from 'react-router-dom';
import { render, screen, waitFor } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

import Link from './';
import { NavigationContext } from '../NavigationContextProvider';
import NavigationWrapper from '../__test-helpers/navigationWrapper';

describe('Link', () => {
const LocationDisplay = () => {
const location = useLocation();

return <div data-testid="location-display">{location.pathname}</div>;
};

let navigationAttemptBlocked = jest.fn(), navigationAttemptUnblocked = jest.fn();

test('navigates to the link route when user clicks it', async () => {
render(<NavigationWrapper>
<Link to="/route" />

<LocationDisplay />
</NavigationWrapper>);

const link = await screen.findByRole('link');
userEvent.click(link);

expect((await screen.findByTestId('location-display'))).toHaveTextContent('/route');
});

test('blocks a navigation attempt when navigation is blocked', async () => {
render(<NavigationWrapper>
<NavigationContext.Provider value={{
isNavigationBlocked: true,
navigationAttemptBlocked,
navigationAttemptResolution: null,
navigationAttemptUnblocked,
}}>
<Link to="/route" />

<LocationDisplay />
</NavigationContext.Provider>
</NavigationWrapper>);

const link = await screen.findByRole('link');
userEvent.click(link);

expect((await screen.findByTestId('location-display'))).toHaveTextContent('/');
});

test('blocks a navigation attempt and unblocks it after when the resolution is true', async () => {
render(<NavigationWrapper>
<NavigationContext.Provider value={{
isNavigationBlocked: true,
navigationAttemptBlocked,
navigationAttemptResolution: true,
navigationAttemptUnblocked,
}}>
<Link to="/route" />

<LocationDisplay />
</NavigationContext.Provider>
</NavigationWrapper>);

const link = await screen.findByRole('link');
userEvent.click(link);

expect((await screen.findByTestId('location-display'))).toHaveTextContent('/');

await waitFor(async () => {
expect((await screen.findByTestId('location-display'))).toHaveTextContent('/route');
});
});
});
52 changes: 50 additions & 2 deletions src/NavigationContextProvider/index.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,59 @@
import React, { createContext, useState } from 'react';
import React, { createContext, useCallback, useState } from 'react';

export const NavigationContext = createContext();

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

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

const [navigationData, setNavigationData] = useState({});

const navigationContextValue = { navigationData, setNavigationData };
const blockNavigation = useCallback(() => setIsNavigationBlocked(true), []);

const cancelNavigationAttempt = useCallback(() => {
if (isNavigationAttemptPending) {
setNavigationAttemptResolution(false);
}
}, [isNavigationAttemptPending]);

const continueNavigationAttempt = useCallback(() => {
if (isNavigationAttemptPending) {
setNavigationAttemptResolution(true);
}
}, [isNavigationAttemptPending]);

const navigationAttemptBlocked = useCallback(() => {
if (isNavigationBlocked) {
setIsNavigationAttemptPending(true);
}
}, [isNavigationBlocked]);

const navigationAttemptUnblocked = useCallback(() => {
setIsNavigationAttemptPending(false);
setNavigationAttemptResolution(null);
}, []);

const unblockNavigation = useCallback(() => {
setIsNavigationAttemptPending(false);
setIsNavigationBlocked(false);
setNavigationAttemptResolution(null);
}, []);

const navigationContextValue = {
isNavigationAttemptPending,
isNavigationBlocked,
navigationAttemptResolution,
navigationData,

blockNavigation,
cancelNavigationAttempt,
continueNavigationAttempt,
navigationAttemptBlocked,
navigationAttemptUnblocked,
unblockNavigation,

setNavigationData,
};

return <NavigationContext.Provider value={navigationContextValue}>
{children}
Expand Down
124 changes: 124 additions & 0 deletions src/NavigationContextProvider/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,128 @@ describe('NavigationContextProvider', () => {

expect((await screen.findByText('"Navigation data!"'))).toBeDefined();
});

test('blocks the navigation', async () => {
const ChildComponent = () => {
const { blockNavigation, isNavigationBlocked } = useContext(NavigationContext);

useEffect(() => {
blockNavigation();
}, [blockNavigation]);

return <p>{`Navigation blocked: ${isNavigationBlocked}`}</p>;
};

render(
<NavigationContextProvider>
<ChildComponent />
</NavigationContextProvider>
);

expect((await screen.findByText('Navigation blocked: true'))).toBeDefined();
});

test('unblocks the navigation', async () => {
const ChildComponent = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taking a look at that 👀

const { blockNavigation, isNavigationBlocked, unblockNavigation } = useContext(NavigationContext);

useEffect(() => {
blockNavigation();
}, [blockNavigation]);

useEffect(() => {
if (isNavigationBlocked) {
unblockNavigation();
}
}, [isNavigationBlocked, unblockNavigation]);

return <p>{`Navigation blocked: ${isNavigationBlocked}`}</p>;
};

render(
<NavigationContextProvider>
<ChildComponent />
</NavigationContextProvider>
);

expect((await screen.findByText('Navigation blocked: false'))).toBeDefined();
});

test('resolves to true when continuing the navigation of a blocked attempt', async () => {
const ChildComponent = () => {
const {
blockNavigation,
continueNavigationAttempt,
isNavigationAttemptPending,
isNavigationBlocked,
navigationAttemptBlocked,
navigationAttemptResolution,
} = useContext(NavigationContext);

useEffect(() => {
blockNavigation();
}, [blockNavigation]);

useEffect(() => {
if (isNavigationBlocked) {
navigationAttemptBlocked();
}
}, [isNavigationBlocked, navigationAttemptBlocked]);

useEffect(() => {
if (isNavigationAttemptPending) {
continueNavigationAttempt();
}
}, [continueNavigationAttempt, isNavigationAttemptPending]);

return <p>{`Navigation attempt resolution: ${navigationAttemptResolution}`}</p>;
};

render(
<NavigationContextProvider>
<ChildComponent />
</NavigationContextProvider>
);

expect((await screen.findByText('Navigation attempt resolution: true'))).toBeDefined();
});

test('resolves to false when canceling the navigation of a blocked attempt', async () => {
const ChildComponent = () => {
const {
blockNavigation,
cancelNavigationAttempt,
isNavigationAttemptPending,
isNavigationBlocked,
navigationAttemptBlocked,
navigationAttemptResolution,
} = useContext(NavigationContext);

useEffect(() => {
blockNavigation();
}, [blockNavigation]);

useEffect(() => {
if (isNavigationBlocked) {
navigationAttemptBlocked();
}
}, [isNavigationBlocked, navigationAttemptBlocked]);

useEffect(() => {
if (isNavigationAttemptPending) {
cancelNavigationAttempt();
}
}, [cancelNavigationAttempt, isNavigationAttemptPending]);

return <p>{`Navigation attempt resolution: ${navigationAttemptResolution}`}</p>;
};

render(
<NavigationContextProvider>
<ChildComponent />
</NavigationContextProvider>
);

expect((await screen.findByText('Navigation attempt resolution: false'))).toBeDefined();
});
});
62 changes: 62 additions & 0 deletions src/NavigationPromptModal/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import React, { memo } from 'react';
import Button from 'react-bootstrap/Button';
import Modal from 'react-bootstrap/Modal';
import PropTypes from 'prop-types';

import { ReactComponent as TrashCanIcon } from '../common/images/icons/trash-can.svg';

import useBlockNavigation from '../hooks/useBlockNavigation';

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

const NavigationPromptModal = ({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

cancelNavigationButtonText,
continueNavigationButtonText,
description,
title,
when,
...rest
}) => {
const { cancelNavigationAttempt, continueNavigationAttempt, isNavigationAttemptPending } = useBlockNavigation(when);

return <Modal {...rest} onHide={cancelNavigationAttempt} show={isNavigationAttemptPending}>
<Modal.Header closeButton>
<Modal.Title>{title}</Modal.Title>
</Modal.Header>

<Modal.Body>{description}</Modal.Body>

<Modal.Footer className={styles.footer}>
<Button className={styles.cancelButton} onClick={cancelNavigationAttempt} variant="secondary">
{cancelNavigationButtonText}
</Button>

<Button
className={styles.continueButton}
onClick={continueNavigationAttempt}
onFocus={(event) => event.target.blur()}
variant="primary"
>
<TrashCanIcon />
{continueNavigationButtonText}
</Button>
</Modal.Footer>
</Modal>;
};

NavigationPromptModal.defaultProps = {
cancelNavigationButtonText: 'Cancel',
continueNavigationButtonText: 'Discard',
description: 'Would you like to discard changes?',
title: 'Discard changes',
};

NavigationPromptModal.propTypes = {
cancelNavigationButtonText: PropTypes.string,
continueNavigationButtonText: PropTypes.string,
description: PropTypes.string,
title: PropTypes.string,
when: PropTypes.bool.isRequired,
};

export default memo(NavigationPromptModal);