feat: add warning on reload and tab exit (#474) - #555
Conversation
aapatil2004
changed the title
aapatil2004
changed the title
alexgarel
left a comment
alexgarel
left a comment
There was a problem hiding this comment.
Thanks @aapatil2004 for the good work. I have some remarks, that are more cleaning up, I imagine.
|
|
||
| useEffect(() => { | ||
| const handleBeforeUnload = (event) => { | ||
| if(taxonomyName || ownerName || branchName || description) { // Show warning only if the form is dirty |
There was a problem hiding this comment.
Not sure but, why don't you use isFormDirty here ?
If it does not work because of dependencies, you may remove its definition.
| useEffect(() => { | ||
| const handleBeforeUnload = (event) => { | ||
| if(taxonomyName || ownerName || branchName || description) { // Show warning only if the form is dirty | ||
| console.log("event warning back triggered"); |
There was a problem hiding this comment.
We should remove console.log before merging.
alexgarel
left a comment
alexgarel
left a comment
There was a problem hiding this comment.
I have another problem with you implementation: it does not work if I click on a link in the application (click on the logo).
Is there a way to also handle it ?
getUserConfirmation (see here) seems the way to go to handle this.
Just to be clear: your implementation does well for part of the cases, we must add the case of application internal navigation (which is also important). |
alexgarel
left a comment
alexgarel
left a comment
There was a problem hiding this comment.
Thanks @aapatil2004 for your work, I have some issues though. I let you give a look at my comments.
| export const OpenAPI: OpenAPIConfig = { | ||
| // TODO: Used to keep API_URL backward compatible with existing code, update API_URL when possible | ||
| BASE: API_URL.substring(0, API_URL.length - 1), | ||
| BASE: typeof API_URL === 'string' ? API_URL.substring(0, API_URL.length - 1) : '', |
There was a problem hiding this comment.
@aapatil2004 this file is auto-generated, so I think this change will be removed on next generation.
I think it's ok because, as seen, it should never be the case that API_URL is not defined. (wee could maybe add a check at another place to have a meaningful error).
You can live it as is in this PR though.
| <FormHelperText | ||
| sx={{ width: "75%", textAlign: "center", maxWidth: "600px" }} | ||
| > | ||
| Explain what is your goal with this new project, what changes are | ||
| you going to bring. Remember to privilege small projects (do big | ||
| project as a succession of small one). | ||
| </FormHelperText> | ||
|
|
| error={isOwnerNameInvalid(ownerName)} | ||
| helperText={ | ||
| isOwnerNameInvalid(ownerName) && | ||
| "Special characters are not allowed" | ||
| } |
There was a problem hiding this comment.
Why did you remove the checks ?
| <FormHelperText | ||
| sx={{ width: "75%", textAlign: "center", maxWidth: "600px" }} | ||
| > | ||
| Please use your Github account username if possible, or eventually | ||
| your id on open food facts slack (so that we can contact you) | ||
| </FormHelperText> | ||
|
|
| error={isInvalidBranchName} | ||
| helperText={ | ||
| isInvalidBranchName && | ||
| "Special characters, capital letters and white spaces are not allowed" | ||
| } |
| return false; | ||
| const handleFieldChange = (setter) => (event) => { | ||
| setter(event.target.value); | ||
| }; |
There was a problem hiding this comment.
this check is important, why was it removed ?
| const handleNavigation = (url: string) => { | ||
| if (taxonomyName || ownerName || description) { |
There was a problem hiding this comment.
We will need to have this kind of confirmation for other pages so we can't check that this way. This check should remain local to the startproject page. Maybe you can do this by first registering a needsConfirm state and check it here, that way we could implement this on more pages with different checks.
Also
| <MenuItem | ||
| key={page.translationKey} | ||
| onClick={handleCloseNavMenu} | ||
| onClick={() => handleNavigation(`/${page.url}`)} // Use handleNavigation |
There was a problem hiding this comment.
I think the method of using BrowserRouter getUserConfirmation proposed in this article might be better (because more central). Did you try it ?
|
@copilot can you fix the merge conflict ? |
3 participants
This is how it looks when we user tries to reload filling partial information
And this is how it looks when user tries to leave the site
Implemented using Windows before unload feature