ui icon indicating copy to clipboard operation
ui copied to clipboard

[Feature] Preventing Closing Tab/Browser And Leaving Page Without Confirmation Modal For Invoices

Open Civolilah opened this issue 1 year ago • 3 comments

@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 INVOLVES preventing 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:

Screenshot 2023-12-18 at 02 08 38

Let me know your thoughts.

Civolilah avatar Dec 14 '23 16:12 Civolilah

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Dec 14 '23 16:12 CLAassistant

While it works, it kinda feels really complex. Can you walk us through the implementation?

Also is it possible to provide test suite for such critical part of the app.

@beganovich I needed to await the merging of some important PRs to resolve conflicts on this one and create tests. The implementation might appear complex, but I believe it can be explained, and it is carried out in few steps.

When a user enters the Invoice pages, they have a few options to leave the page: close the browser, close the tab, use the back browser button, navigate using the left main navigation, use the Quick Creation Popover in the navigation, use the back button in the navigation, or use actions on the Invoice entity.

To prevent the browser/tab from closing, we need to add a beforeunload event in the app. This event will track these actions, and under the function called handleBeforeUnload, there is a small logic defining when the default browser's modal should be displayed to prevent the user from leaving the page.

To prevent the user from leaving the page using the back browser's button, we need to use the popstate event. This event is triggered when the user clicks that button, but we are unable to disable the button or prevent navigation. Instead, we need to push a new state into the history object to prevent navigation. I explained much more about this in the comment you asked for regarding the actionKey property.

All other preventions are handled using the main usePreventNavigation hook. If prevention is necessary, the hook will know it using the preventLeavingPageAtom atom. The state of this atom changes while the user changes the invoice object, allowing us to compare the first version when the user joined the page and the version while they have been making changes.

Once the preventNavigation function is triggered, we store the necessary details such as the URL, external URL (bool), actionKey, or probably the function because some actions are not only based on navigation, we need to execute some part of the logic.

If the user clicks "continue editing," nothing will change except the modal, which will be closed. If the user clicks "discard changes," then the logic will be executed under the PreventNavigationModal based on the stored details.

Of course, there is also a case when we do not need prevention. In this case, the preventNavigation function will execute the same action as needed by default.

Literally, everything is implemented in the main components—DropdownElement, Link. There are only, I believe, 2-3 very rare cases when those components are not sufficient.

Of course, I encountered some cases where the components are used directly from the package, not our custom components, such as the Link component in the Breadcrumbs. There, I needed to make a change to use the default component so that the entire logic can be globalized. Cases like this one are probably the main reason why the PR is a bit larger and seems complex.

The entire logic is globalized, so the implementation of this functionality for any other entity is a minor part of the job. In the logic of Creation and Edition components, only a part of the code that tracks whether navigation should be prevented needs to be added.

Also, for the main parts of the functionality, such as the navigation bar, main left navigation, or invoice actions, tests have been implemented.

Let me know your thoughts.

Civolilah avatar Dec 27 '23 00:12 Civolilah

@beganovich we'll need to look into this and deploy a solution.

turbo124 avatar May 08 '24 21:05 turbo124

on review, it appears only certain fields are intercepted.

the line items and any data within the text area's are not triggering the modal.

turbo124 avatar May 14 '24 05:05 turbo124

on review, it appears only certain fields are intercepted.

the line items and any data within the text area's are not triggering the modal.

@turbo124 Yeah, I can recreate it, it only happens with text area input fields. I will fix it.

Civolilah avatar May 14 '24 17:05 Civolilah

@Civolilah please mark this as QA pending and ready for review once it's ready.

beganovich avatar May 22 '24 16:05 beganovich

@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.

Civolilah avatar May 26 '24 19:05 Civolilah

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 avatar May 26 '24 21:05 turbo124

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.

Civolilah avatar May 27 '24 00:05 Civolilah