-
Notifications
You must be signed in to change notification settings - Fork 14
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
Alert user when they leave without saving #408
Conversation
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.
Nice, works for the most part. I see what you mean about react router now.
I ran into a couple issues. When first visiting the roadmap page, if I try to leave without interacting with the planner at all it shows the unsaved changes alert. I think the problem here is that the upon loading the roadmap from mongo or local storage, this useEffect is called a second time.
peterportal-client/site/src/pages/RoadmapPage/Planner.tsx
Lines 42 to 54 in 3c7c8da
useEffect(() => { | |
// if is first render, load from local storage | |
if (isFirstRenderer) { | |
loadRoadmap(); | |
} | |
// validate planner every time something changes | |
else { | |
validatePlanner(); | |
// mark changes as unsaved to alert before leaving page | |
dispatch(setUnsavedChanges(true)); | |
} | |
}, [data, transfers]); |
Another thing we may want to take into consideration is that if you make a change then undo it, say add a class then remove it, there's still considered to be unsaved changes. Maybe an approach that can resolve these two would be to compare the current state of the roadmap with its previous save.
Switching to the catalogue page with unsaved changes and then attempting to close the tab seems to show the alert appropriately. There appears to be an issue though where if you switch to the catalogue, or another page, then switch back, unsaved changes are lost. This is due to the roadmap loading data again and overwriting any edits to the current state. We would only want it to load data if there's none in the redux slice, not every time the component is mounted. I think I'll put up a separate issue for that.
Changed so that on roadmap edit, check whether the current roadmap is equal to the last-saved roadmap in local storage, and only mark as unsaved if they are different. Not sure to fix the last issue in your comment though |
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.
Nice. For my last issue, I think replacing the isFirstRenderer in the planner's useEffect with a check if the current roadmap is empty would work.
That hook could also be deleted entirely in that case since it is used nowhere else.
Co-authored-by: Jacob Sommer <[email protected]>
Removing Also, now that I think about it more, having a toast pop up every time the tab is switched would be pretty annoying. The user doesn't really need to be notified constantly that their changes are unsaved. The alert will still show up when the user exits the site from the catalogue tab. |
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.
Ok, looks good! Just left one suggestion.
Co-authored-by: Jacob Sommer <[email protected]>
Deployed staging instance to https://staging-408.peterportal.org |
Description
Keep track in state of when there are unsaved changes (mark true when planner is validated, and mark false when save button is clicked). Use the onbeforeunload event listener when unsaved changes is true, which shows an alert when the user leaves the page, closes the window, or reloads. Was unable to use useBlocker because of the following issue:
Only the new routers e.g. createBrowserRouter are compatible with useBlocker, see https://reactrouter.com/en/6.21.3/routers/picking-a-router. I didn't want to mess with the existing router, so I used vanilla JS listeners which work fine in this case.
Screenshots
https://www.loom.com/share/5822237a4a5c428cace0d954b225eb8a?sid=0e1e2bc3-38b4-4e8e-921a-348587ae86a9
Steps to verify/test this change:
Final Checks:
Issues
Closes #303