Save changes popup #1859
Save changes popup #1859
Conversation
|
Deployed to https://pr-1859.aam-digital.net/ |
There was a problem hiding this comment.
From a usability perspective:
- confirmation shows while in the background navigation is already done. This seems uncommon for such a workflow, feels slightly off (dialog refers to previous view) and it may be useful for a user to still see the changes to save/discard.
Navigating to Children list and confirming to save changes, the list does not get updated with the newly saved change (e.g. name is still the old one until reloading that view)(not applicable anymore, as navigation only when discarding)- Should we ask for confirmation also when clicking "Cancel" instead of "Save" in the Form? (We do that on "Cancel" in the popup forms already)
- Popup form "Save" button still asks whether I want to save changes. That shouldn't be necessary.
- Cancel (in addition to saving yes/no) would be a useful further option to go back to editing. Half the time I get such a message in other software I end up doing some further editing. (Could be moved to a separate PR/issue if we want this out quickly)
- some websites prevent you to navigate away even to a completely different domain in the browser if you have unsaved changes. This could be useful here as well. Currently I can navigate or reload to lose changes and the confirmation only happens for internal app navigation
|
@sleidig I have now implemented the logic in a way that it shows the form popup before navigation is done. With this approach there are still some open questions:
|
src/app/core/form-dialog/dialog-buttons/dialog-buttons.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
good improvements! :-)
What do we do with different panels (we could deactivate the other panels while one is in edit mode)?
More comfortable would be if we can consider switching tabs the same as switching pages and show the dialog instead of preventing the switch per se. Technically difficult?
When do we want to show the popup? At the moment it only occurs on navigation and when clicking the backdrop of a popup. Further options would also include "Cancel" and the "X" button in a popup.
Slightly unsure here. What do you think? Tending a bit towards showing it on cancel, too.
Recently read a design book that highlighted how the safeguard ("Do you want to save changes?") can become the most easy, default way to save a document for some users. In that regard, showing the dialog (and maybe considering to extend it with even more explicit buttons) could be worth considering.
src/app/core/entity-components/entity-details/form/unsaved-changes.service.ts
Outdated
Show resolved
Hide resolved
…nges.service.ts Co-authored-by: Sebastian <sebastian.leidig@gmail.com>
…anges_popup # Conflicts: # src/app/core/entity-components/entity-details/form/unsaved-changes.service.ts
|
The features that have not been implemented yet are very difficult to achieve: Restrict panel navigation with unsaved changesIt is not possible to keep the panel activated but still disable the navigation to another panel. We can either disable the panel completely or only show a popup after the navigation has happened. In the later case it's still difficult to implement the "leave/cancel" functionality as this would need to be communicated back to the form component (at the moment it would just stay in edit mode). Having a save button in the popupThis has the same problem as mentioned before that the "save" trigger needs to be forwarded to the underlying component which is quite difficult to achieve with our setup. |
There was a problem hiding this comment.
Tested:
details view (in edit mode with changes):
- navigate -> confirmation | yes -> navigating and discarding changes
- navigate -> confirmation | no -> remain on view in edit mode
- [edit mode without changes] -> navigate -> no confirmation dialog
- close browser / leave app -> browser confirmation | cancel -> remain on view in edit mode
-
switch tab -> ❓ no confirmation -> navigate -> :warn: confirmation (about unfocused tab)
-> I agree with your suggestion and thinking about would now vote for disabling tab switches while in edit mode. This would also be consistent with our UX when creating a new entity (and we can maybe use the same code).
popup form (with changes):
- backdrop click -> confirmation | no -> remain on popup with changes
- backdrop click -> confirmation | yes -> closing popup, discarding changes (:rotating_light: changes are currently saved anyway)
- cancel click -> no confirmation
⚠️ I would've expected that cancel and backdrop have the same behavior but it also makes sense to not additionally ask on cancel (and that is consistent with the details view behavior) ✔️ - [without changes] -> cancel/backdrop -> no confirmation dialog
- close browser / leave app -> browser confirmation | cancel -> remain on view in edit mode
⚠️ not sure whether technically not possible but for UX + consistency it would be great to prevent browser close in this case as well
src/app/core/entity-components/entity-details/form/form.component.ts
Outdated
Show resolved
Hide resolved
src/app/core/entity-components/entity-details/form/form.component.ts
Outdated
Show resolved
Hide resolved
src/app/core/entity-components/entity-details/form/unsaved-changes.service.spec.ts
Outdated
Show resolved
Hide resolved
src/app/core/entity-components/entity-details/form/unsaved-changes.service.ts
Show resolved
Hide resolved
src/app/core/confirmation-dialog/confirmation-dialog.service.ts
Outdated
Show resolved
Hide resolved
# Conflicts: # src/app/core/form-dialog/dialog-buttons/dialog-buttons.component.spec.ts
There was a problem hiding this comment.
Thanks for the code clarifications! 🙏 Looks excellent now.
I re-tested and updated the checklist above: #1859 (review)
Good to go for me.
|
Kudos, SonarCloud Quality Gate passed!
|
|
🎉 This PR is included in version 3.22.0-master.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 3.22.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
see issue: #1424
Visible/Frontend Changes
Architectural/Backend Changes
--