-
Notifications
You must be signed in to change notification settings - Fork 95
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
FIX Reinstate unsaved changes dialog #1458
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ import $ from 'jquery'; | |
import React from 'react'; | ||
import { Provider as ReduxProvider } from 'react-redux'; | ||
import ReactDOM from 'react-dom/client'; | ||
import { BrowserRouter } from 'react-router-dom'; | ||
import { createBrowserRouter, createRoutesFromElements, RouterProvider } from 'react-router-dom'; | ||
import Config from 'lib/Config'; | ||
import pageRouter from 'lib/Router'; | ||
import reactRouteRegister from 'lib/ReactRouteRegister'; | ||
|
@@ -15,8 +15,9 @@ import { ApolloProvider } from '@apollo/client'; | |
import i18n from 'i18n'; | ||
import { isDirty } from 'redux-form'; | ||
import getFormState from 'lib/getFormState'; | ||
import { Routes, Route } from 'react-router'; | ||
import { Route } from 'react-router'; | ||
import { joinUrlPaths } from 'lib/urls'; | ||
import NavigationBlocker from '../components/NavigationBlocker/NavigationBlocker'; | ||
|
||
/** | ||
* Bootstraps routes | ||
|
@@ -35,8 +36,7 @@ class BootRoutes { | |
const base = Config.get('absoluteBaseUrl'); | ||
pageRouter.setAbsoluteBase(base); | ||
|
||
this.handleBeforeRoute = this.handleBeforeRoute.bind(this); | ||
this.handleBeforeUnload = this.handleBeforeUnload.bind(this); | ||
Comment on lines
-38
to
-39
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. Neither of these functions is passed into downstream components anymore, so we don't need explicit binds. |
||
this.shouldConfirmBeforeUnload = this.shouldConfirmBeforeUnload.bind(this); | ||
} | ||
|
||
setStore(store) { | ||
|
@@ -109,18 +109,30 @@ class BootRoutes { | |
initReactRouter() { | ||
reactRouteRegister.updateRootRoute({ component: App }); | ||
const rootRoute = reactRouteRegister.getRootRoute(); | ||
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> | ||
Comment on lines
-112
to
137
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. Ultimately the only differences here are:
See #1458 (comment) for the reason we had to change the way we're doing the browserrouter etc. |
||
); | ||
|
@@ -134,12 +146,7 @@ class BootRoutes { | |
const store = this.store; | ||
|
||
pageRouter('*', (ctx, next) => { | ||
const msg = i18n._t( | ||
'Admin.CONFIRMUNSAVED', | ||
`Are you sure you want to navigate away from this page?\n\n | ||
WARNING: Your changes have not been saved.\n\n | ||
Press OK to continue, or Cancel to stay on the current page.` | ||
); | ||
const msg = this.getUnsavedChangesMessage(); | ||
if (!this.shouldConfirmBeforeUnload() || window.confirm(msg)) { | ||
// eslint-disable-next-line no-param-reassign | ||
ctx.store = store; | ||
|
@@ -218,22 +225,10 @@ class BootRoutes { | |
return changedForms.length > 0; | ||
} | ||
|
||
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); | ||
Comment on lines
-221
to
-236
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. Neither of these is needed anymore |
||
getUnsavedChangesMessage() { | ||
return 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.`); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import { useEffect } from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import { unstable_useBlocker as useBlocker } from 'react-router-dom'; | ||
|
||
export default function NavigationBlocker({ shouldBlockFn, blockMessage }) { | ||
const blocker = useBlocker(shouldBlockFn); | ||
useEffect(() => { | ||
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. Has to be in an effect to avoid the console error I mentioned in #1458 (comment) |
||
if (blocker.state === 'blocked') { | ||
// eslint-disable-next-line no-alert | ||
const canUnload = confirm(blockMessage); | ||
if (canUnload) { | ||
blocker.proceed(); | ||
} else { | ||
blocker.reset(); | ||
} | ||
} | ||
}, [blocker.state]); | ||
|
||
// Don't actually render anything - this component exists purely to hold the blocker | ||
// logic, which we put here to (hopefully) avoid rerendering the entire app when blocker | ||
// state changes. | ||
return null; | ||
} | ||
|
||
NavigationBlocker.propTypes = { | ||
shouldBlockFn: PropTypes.func.isRequired, | ||
blockMessage: PropTypes.string.isRequired, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,8 +6,8 @@ import { Outlet } from 'react-router'; | |
* Empty container for the moment, will eventually contain the CMS menu` | ||
* and apply to document.body, rather than just one specific DOM element. | ||
*/ | ||
const App = () => ( | ||
<div className="app"><Outlet /></div> | ||
const App = ({ children }) => ( | ||
<div className="app">{children}<Outlet /></div> | ||
Comment on lines
+9
to
+10
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. We have to have the blocker as a child of the root component because it has to be a child of the |
||
); | ||
|
||
export default provideInjector(App); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,8 +71,8 @@ | |
"react-dom": "^18.2.0", | ||
"react-load-script": "^0.0.6", | ||
"react-redux": "^8.0.4", | ||
"react-router": "^6.4.2", | ||
"react-router-dom": "^6.4.2", | ||
"react-router": "^6.7", | ||
"react-router-dom": "^6.7", | ||
Comment on lines
-74
to
+75
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 new |
||
"react-select": "^5.5.8", | ||
"reactstrap": "^8.9.0", | ||
"redux": "^4.2.0", | ||
|
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.
We need to use
createBrowserRouter()
andcreateRoutesFromElements()
instead of adding theBrowserRouter
andRoutes
components ourselves because theRouterProvider
doesn't like us doing it ourself - and we have to use theRouterProvider
because this instantiates a "data router" which is necessary to use the newuseBlocker
hook.