[Google Blockly] Add delete confirmation modal to the modal function editor #54461
Conversation
| isDangerCancel: true, | ||
| confirmText: msg.keep(), | ||
| onConfirm: null, | ||
| onCancel: this.onDeleteConfirmed.bind(this), |
There was a problem hiding this comment.
Is this backward? I'd expect confirmText and onConfirm to handle the deletion and cancelText and onCancel to handle bailing out of the deletion.
Also, why is there no handler for onConfirm?
There was a problem hiding this comment.
It looks like you had to swap them to retain the order of the buttons relative to CDO. I think it's worth, at least, leaving a comment about why it's the opposite of what one would expect. I also wonder if it's worth asking Mark whether the OG button ordering is best (this could be done in a follow-up PR).
There was a problem hiding this comment.
The reason for this is it allows us to have a red delete button and a purple keep button. I'll add a comment explaining this. The reason there is no onConfirm handler is we do nothing if they want to keep the function.
There was a problem hiding this comment.
Added comments. We can follow-up on if we should switch up the order/color, but I don't think that's blocking given no one has complained about the existing modal.
There was a problem hiding this comment.
I think the thing I was missing re: the handler was that the modal closes regardless of which button you push -- this is hidden in the modal code, presumably.
|
@molly-moen I checked with Mark and confirmed the buttons are how we want them. No follow-up. |
Add a delete confirmation modal to the new modal function editor. We can reuse the same modal that we use in cdo blockly, as we were already passing it through the Google Blockly Wrapper. It is set here and passed through here.
The dialog looks identical to the old dialog:
Links
Testing story
Tested locally that now when you click delete you get the dialog, and only on delete confirmation do you actually delete the function.
PR Checklist: