-
Notifications
You must be signed in to change notification settings - Fork 0
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
Changes from 4 commits
f09d3b1
a006fd7
2f04614
08e54c1
55195cd
638130d
a808fc1
b29c891
69a0324
c89410a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 }) => { | ||
const { | ||
isNavigationBlocked, | ||
navigationAttemptBlocked, | ||
navigationAttemptResolution, | ||
navigationAttemptUnblocked, | ||
} = useContext(NavigationContext); | ||
|
||
const linkRef = useRef(); | ||
|
||
const [localNavigationAttemptBlocked, setLocalNavigationAttemptBlocked] = useState(false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an event handler with no |
||
} 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; |
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'); | ||
}); | ||
}); | ||
}); |
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 = () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm seeing some funky test patterns here. For example, in Since you're only really testing output values from There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
}); | ||
}); |
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 = ({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New prompt component that automatically blocks the navigation (if |
||
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); |
There was a problem hiding this comment.
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 normalLink