FIX Reinstate unsaved changes dialog #1458
Conversation
|
UPDATE: The problem mentioned in this comment is resolved. Leaving here for historical context only. I've got this to a state where it technically meets the acceptance criteria - except after the initial "yes continue I want to navigate away" it keeps prompting you every time you go somewhere until you go back to the original form. It seems the state of the form is stuck as dirty until you prove (by navigating back to it) that you don't have unsaved changes there anymore. There's a console error which I think is part of the problem when you accept the prompt the first time:
I'm pretty sure react isn't able to update the form state at the same time as the blocker state, and for whatever reason the blocker state gets prioritised, so yes the blocker lets you continue, but each time it asks "should I block this new navigation?" the app tells it "yes, the form is still dirty" |
This broke when upgrading to react-router 6.4 - they've added a new API for it in 6.7
GuySartorelli
force-pushed
the
pulls/2.0/unsaved-changes-dialog
branch
from
97d5483 to
d5014f1
Compare
| import { BrowserRouter } from 'react-router-dom'; | ||
| import { createBrowserRouter, createRoutesFromElements, RouterProvider } from 'react-router-dom'; |
There was a problem hiding this comment.
We need to use createBrowserRouter() and createRoutesFromElements() instead of adding the BrowserRouter and Routes components ourselves because the RouterProvider doesn't like us doing it ourself - and we have to use the RouterProvider because this instantiates a "data router" which is necessary to use the new useBlocker hook.
| this.handleBeforeRoute = this.handleBeforeRoute.bind(this); | ||
| this.handleBeforeUnload = this.handleBeforeUnload.bind(this); |
There was a problem hiding this comment.
Neither of these functions is passed into downstream components anymore, so we don't need explicit binds.
| const routes = reactRouteRegister.getChildRoutes().map((route) => ( | ||
| <Route key={route.path} path={route.path} element={<route.component />} /> | ||
| )); | ||
| const router = createBrowserRouter( | ||
| createRoutesFromElements( | ||
| <Route | ||
| path={rootRoute.path} | ||
| element={ | ||
| <rootRoute.component> | ||
| <NavigationBlocker shouldBlockFn={this.shouldConfirmBeforeUnload} blockMessage={this.getUnsavedChangesMessage()} /> | ||
| </rootRoute.component> | ||
| } | ||
| > | ||
| {reactRouteRegister.getChildRoutes().map( | ||
| (route) => ( | ||
| <Route key={route.path} path={route.path} element={<route.component />} /> | ||
| ) | ||
| )} | ||
| </Route> | ||
| ), | ||
| { basename: joinUrlPaths(Config.get('baseUrl'), Config.get('adminUrl')) } | ||
| ); | ||
|
|
||
| ReactDOM.createRoot(document.getElementsByClassName('cms-content')[0]).render( | ||
| <ApolloProvider client={this.client}> | ||
| <ReduxProvider store={this.store}> | ||
| <BrowserRouter basename={joinUrlPaths(Config.get('baseUrl'), Config.get('adminUrl'))}> | ||
| <Routes> | ||
| <Route path={rootRoute.path} element={<rootRoute.component />}>{routes}</Route> | ||
| </Routes> | ||
| </BrowserRouter> | ||
| <RouterProvider router={router} /> | ||
| </ReduxProvider> | ||
| </ApolloProvider> |
There was a problem hiding this comment.
Ultimately the only differences here are:
- We now have a
RouterProvider(which is necessary for the newuseBlockerhook) - We're passing a component in as a child of the
Appcomponent (rootRoute.component)
See #1458 (comment) for the reason we had to change the way we're doing the browserrouter etc.
| handleBeforeUnload(content, callback) { | ||
| if (this.shouldConfirmBeforeUnload()) { | ||
| return callback(confirm(i18n._t('Admin.CONFIRMUNSAVEDSHORT', 'WARNING: Your changes have not been saved.'))); | ||
| } | ||
|
|
||
| return callback(true); | ||
| } | ||
|
|
||
| handleBeforeRoute(content, callback) { | ||
| if (this.shouldConfirmBeforeUnload()) { | ||
| return callback(confirm(i18n._t('Admin.CONFIRMUNSAVED', `Are you sure you want to navigate away | ||
| from this page?\n\nWARNING: Your changes have not been saved.\n\n | ||
| Press OK to continue, or Cancel to stay on the current page.`))); | ||
| } | ||
|
|
||
| return callback(true); |
There was a problem hiding this comment.
Neither of these is needed anymore
|
|
||
| export default function NavigationBlocker({ shouldBlockFn, blockMessage }) { | ||
| const blocker = useBlocker(shouldBlockFn); | ||
| useEffect(() => { |
There was a problem hiding this comment.
Has to be in an effect to avoid the console error I mentioned in #1458 (comment)
| const App = ({ children }) => ( | ||
| <div className="app">{children}<Outlet /></div> |
There was a problem hiding this comment.
We have to have the blocker as a child of the root component because it has to be a child of the RouteProvider and this is the only component we know for sure will be in there.
But because this replaceable via the reactRouteRegister we don't even know that the root component will specifically be App so we can't just slap it directly here, we have to pass it in as a child prop.
| "react-router": "^6.4.2", | ||
| "react-router-dom": "^6.4.2", | ||
| "react-router": "^6.7", | ||
| "react-router-dom": "^6.7", |
There was a problem hiding this comment.
The new useBlocker hook was only introduced in 6.7 (I'll update the changelog as well to reflect the new dependency constraint)
This broke when upgrading to react-router 6.4 - they've added a new API for it in 6.7
I've added behat tests for this in asset-admin: silverstripe/silverstripe-asset-admin#1335
Notes
beforeunloadevent. Adding a custom message to it is not supported by modern browsers.Parent issue