[Feature] Preventing Closing Tab/Browser And Leaving Page Without Confirmation Modal For Invoices #1419
Conversation
|
|
src/App.tsx
Outdated
| interface PreventLeavingPage { | ||
| prevent: boolean; | ||
| actionKey?: 'switchCompany' | 'browserBack'; | ||
| } | ||
|
|
||
| interface HistoryLocation { | ||
| lastLocation: string; | ||
| nonPreventedLocations: string[]; | ||
| } | ||
|
|
||
| export const preventLeavingPageAtom = atom<PreventLeavingPage>({ | ||
| prevent: false, | ||
| actionKey: undefined, | ||
| }); | ||
|
|
||
| export const lastHistoryLocationAtom = atom<HistoryLocation>({ | ||
| lastLocation: '', | ||
| nonPreventedLocations: [], | ||
| }); |
There was a problem hiding this comment.
app.tsx is getting quite chunky, please move this into a separate module or usePreventNavigation, that works, too.
There was a problem hiding this comment.
@beganovich Yeah, I separated it into the hook called useAddPreventNavigationEvents.
src/App.tsx
Outdated
|
|
||
| interface PreventLeavingPage { | ||
| prevent: boolean; | ||
| actionKey?: 'switchCompany' | 'browserBack'; |
There was a problem hiding this comment.
What exactly is actionKey and what it is used for?
There was a problem hiding this comment.
@beganovich We have two cases when the actionKey is needed. The actionKey is a property that defines what is currently triggered in our case—either the switch company functionality or triggering the back button on the browser.
So, why is it important?!
The functionality of switching companies is based on the browser reloading, which immediately means that if needed at that moment, the beforeunload event will be triggered, and the browser will show the default modal for preventing leaving the page. This modal provides 'Cancel' and 'Leave' buttons, but in the case of switching companies, 'Cancel' will work just as we need. However, the 'Leave' button will not know how to switch the company (it will not remember the 'switchCompany' function and execute it as needed). Therefore, we need to display our own modal and also prevent the default one from being displayed. Essentially, the 'switchCompany' actionKey is only used when it is necessary to prevent the display of the default browser's modal.
The second 'browserBack' action key is used when it is necessary to push a new state into the history. I conducted thorough research, and it is impossible to disable the back button of the browser unless we push the current page into the state. This way, when the user clicks the back button, they will stay on that page. So, this actionKey is used when we need to do that, avoiding unnecessary pushing of a new state into the history. Additionally, there is another case when it is also very important. If the user triggers the back button and then triggers the 'continue editing' button on the prevent navigation modal multiple times in a row (potentially), they will have, let's say, 10+ instances of the current page in the history object. Therefore, when the prevention of navigation is no longer needed, the user would have to click the back button 10+ times in the browser to go back to the previously non-prevented page. To avoid this poor UX, I store the last page once the user is not prevented from navigation, and then I navigate the user to that page instead of navigating to the exact same page where they are at that moment.
Please let me know if you need any additional explanation for this part of the logic.
| const handleDiscardChanges = () => { | ||
| const isBrowserBackAction = preventLeavingPage.actionKey === 'browserBack'; | ||
|
|
||
| const { url, externalLink, fn } = blockedNavigationAction || {}; | ||
|
|
||
| if (!externalLink) { | ||
| setLastHistoryLocation((current) => ({ | ||
| ...current, | ||
| lastLocation: '', | ||
| })); | ||
| setPreventLeavingPage({ prevent: false, actionKey: undefined }); | ||
| } | ||
|
|
||
| setIsNavigationModalVisible(false); | ||
|
|
||
| const numberOfNonPreventedLocations = nonPreventedLocations.length; | ||
|
|
||
| let lastNonPreventedLocation = | ||
| nonPreventedLocations[numberOfNonPreventedLocations - 1]; | ||
|
|
||
| lastNonPreventedLocation = | ||
| lastNonPreventedLocation !== location.pathname | ||
| ? lastNonPreventedLocation | ||
| : nonPreventedLocations[numberOfNonPreventedLocations - 2]; | ||
|
|
||
| if (isBrowserBackAction && lastNonPreventedLocation) { | ||
| navigate(lastNonPreventedLocation); | ||
| } | ||
|
|
||
| if (blockedNavigationAction) { | ||
| if (url) { | ||
| if (url === 'back') { | ||
| lastNonPreventedLocation && navigate(lastNonPreventedLocation); | ||
| } else { | ||
| if (externalLink) { | ||
| window.open(url, '_blank'); | ||
| } else { | ||
| navigate(url); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn?.(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
This looks fairly complex. Any chance we can provide some tests for this one? We definitely don't want navigation just randomly failing, that'd be bad.
There was a problem hiding this comment.
@beganovich Sure, explaining the entire logic in this comment: #1419 (review)
@turbo124 Yeah, I can recreate it, it only happens with text area input fields. I will fix it. |
|
@Civolilah please mark this as QA pending and ready for review once it's ready. |
|
@beganovich @turbo124 The PR now includes the fixes we discussed, along with fixes to the tests so we can run them locally at any time to check for any issues. The PR is ready for review. Let me know your thoughts. |
|
tinymce input is not being triggered for this action. also need to ensure any inputs in text area in the line items are also captured. |
@turbo124 Ah, great catch! I was able to recreate it. The issue was with the reference. Now, the issue should be fixed, and I cannot recreate it anymore. It should work for any input field, including the TinyMCE markdowns and any text area input fields as you mentioned. Let me know if you also see it fixed. |
| ? new InvoiceSumInclusive({ ...invoice }, currency).build() | ||
| : new InvoiceSum({ ...invoice }, currency).build(); |
There was a problem hiding this comment.
I wonder if this is still required? Isn't the spread operator part of previous implementation?
There was a problem hiding this comment.
@beganovich Yes, you are right, this is unnecessary until we improve the implementation. So, I've removed the spreading. Along with this removal, I've fixed conflicts related to merging other PRs. Let me know your thoughts.
@beganovich @turbo124 The PR includes the implementation of preventing users from leaving the Creation and Edition pages for Invoices if changes have been made to the corresponding Invoice for which the page is open. It
INVOLVESpreventing users from leaving the page through the app buttons/links (navigating to another app page), preventing the closure of the tab or the entire browser, preventing navigation back in the history using the back browser button, preventing direct entry of URLs in the browser, and preventing page reloading. Screenshot:Let me know your thoughts.