Modal unsaved work confirmation #7
Conversation
blackbaud-conorwright
requested a review
from Blackbaud-SteveBrush
as a code owner
Codecov Report
@@ Coverage Diff @@
## master #7 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 21 22 +1
Lines 430 439 +9
Branches 64 66 +2
=====================================
+ Hits 430 439 +9
Continue to review full report at Codecov.
|
|
Added blackbaud/skyux2#2138 to document new input. |
There was a problem hiding this comment.
Ok. I have 4 thoughts that I'd like your thoughts on.
- I think we should probably have a seperate demo for just this and revert the old full page demo to how it was. That way people know that they should look at that given example for how to show the confirmation. In the current state it is hard to tell where you need to look and if someone is just looking at the fullpage then they might be confused when the confirmation pops up.
- I'd like to hear feedback from Todd on the default text
- I think it would be good if we gave another input that allows for custom text for both the modal body and the buttons. I feel like design/docs will want specific text for some apps here and so we should try to get ahead of that request
- I think I agree with Steve that we should possibly make the input to trigger the confirmation more generic. I think we will find out that people want this confirmation for more than just unsaved work and so I would rather name the input something such as
triggerConfirmationthat is a little more generic.
|
@Blackbaud-TrevorBurch This should be ready for another look now 👍 |
|
Aside from the message though, we still need to wait on Todd to get back for that feedback. But that won't really change the implementation, just the default message |
src/app/public/modules/modal/close-confirmation/modal-close-confirm-configuration.ts
Outdated
Show resolved
Hide resolved
|
@Blackbaud-TrevorBurch This should be set for another look. |
There was a problem hiding this comment.
@blackbaud-conorwright Regarding the issue of having a circular dependency on confirm: I think this unearthed a bigger problem. The goal of this feature is to allow consumers the opportunity to short-circuit the closing of a modal, run a check, and either keep it open, or close it. We could be overthinking the implementation. What if we removed the generating of the "confirm" aspect entirely, and only fired an event on the modal instance instead. The consumer of the modal would subscribe to the beforeClose lifecycle hook and generate a confirm instance themselves.
(NOTE: I'm not 100% sold on the names of the types, below. Curious if you have some ideas, there.)
An example consumer component:
private hasUnsavedWork = false;
// ...
const modal = this.modalService.open(SomeComponent);
modal.beforeClose.subscribe((handler: SkyModalBeforeCloseHandler) => {
if (!this.hasUnsavedWork) {
handler.triggerClose();
return;
}
// Unsaved work detected. Alert the user.
const confirm = this.confirmService.open({ message: 'Are you sure?' });
confirm.closed.subscribe((result: any) => {
if (result.action === 'yes') {
handler.triggerClose();
} else {
handler.abortClose();
}
});
});
modal.closed.subscribe(() => ...);
The benefits of this approach:
- The consumer can use the full suite of Confirm features that they require (and can even create a custom confirm if they need to).
- The complexity of this feature is significantly reduced.
- We can build on this idea later on with different lifecycle hooks (if needed).
|
@Blackbaud-SteveBrush The new implementation is up |
src/app/public/modules/modal/types/modal-before-close-handler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
LGTM. I'd appreciate it if @Blackbaud-TrevorBurch could give it one more glance.
There was a problem hiding this comment.
@blackbaud-conorwright sorry about this but I did spot some minor style things.
|
No problem! I'm happy to fix style stuff :) |
Resolves: blackbaud/skyux2#1415