Add ability to pass a title and show the close button on a modal #10069

stacey-gammon · 2017-01-25T19:18:16Z

Nothing is using this yet, but view/edit mode will need the concept of three separate actions - close (do nothing), use current filters, and load dashboard filters.

w33ble · 2017-01-25T22:18:49Z

src/ui/public/modals/confirm_modal.js

@@ -23,9 +27,15 @@ module.factory('confirmModal', function ($rootScope, $compile) {
  return function confirmModal(message, customOptions) {
    const defaultOptions = {
      onCancel: noop,
-      cancelButtonText: 'Cancel'
+      cancelButtonText: 'Cancel',
+      onClose: noop,


I think that in every case, onCancel and onClose will reference the same handler (assuming onCancel is a handler they need in the first place).

This also introduces a gotcha where a user can forget to add an onCancel handler even in situations where they DID add an onCancel, but the dialog still works and now they have a silent error condition.

3 handlers on a dialog with 2 buttons seems strange. I think when a user clicks the close button, they mean to do the same thing as when they click the cancel button, so calling onCancel on close seems appropriate, and leaving this at 2 handlers seems right.

This also introduces a gotcha where a user can forget to add an onCancel handler even in situations where they DID add an onCancel,

I'm not sure what you mean by this.

View/edit mode needs this situation. It has two choices and I also want to give the ability for the user to "x" out of the dialog and "do nothing". I suppose I could not accept a custom onClose function and always make it a noop, which would be sufficient for my needs.

These are the two dialogs that I want to use this for:
Here 'x' will cancel out jumping into edit mode:

Here 'x' will cancel out leaving edit mode:

Actually I have an idea. How about if the user passes a custom onCancel but not a custom onClose, and passes showClose, onClose defaults to onCancel instead of noop?

I think Stacey's suggestion is the best option for now.

Longer term, I don't think ConfirmModal is the right tool for this use case. I think we should only use ConfirmModal for yes/no questions and create different modals for situations like this one.

That last example isn't a confirm dialog. There shouldn't be 3 choices on a confirmation. Confirm is a yes/no, not a yes/no/exit.

For this to be a confirmation dialog, the flow should just say something like "You will lose your changes, are you sure?" with a "Lose Changes" option and a "Go back to edit" option, and they can save manually.

Otherwise, I think this needs a new kind of modal which allows for 3 actions, Lose Changes, Save Changes, and Back to Edit.

cjcenizal

I love these tests! I had a few suggestions.

cjcenizal · 2017-01-26T17:22:50Z

src/ui/public/modals/__tests__/confirm_modal.js

+        expect(true).to.be(false);
+      } catch (error) {
+        expect(error).to.not.be(undefined);
+      }


According to the docs we can make assertions about errors like this:

expect(fn).to.throw(); // synonym of throwException expect(fn).to.throwError(); // synonym of throwException expect(fn).to.throwException(function (e) { // get the exception object expect(e).to.be.a(SyntaxError); }); expect(fn).to.throwException(/matches the exception message/); expect(fn2).to.not.throwException();

So we can rewrite this test to be:

expect(() => confirmModal('hi', { onConfirm: _.noop })).to.throw("Error message here");

Fancy! Now that looks much better.

cjcenizal · 2017-01-26T17:23:06Z

src/ui/public/modals/__tests__/confirm_modal.js

+        expect(true).to.be(false);
+      } catch (error) {
+        expect(error).to.not.be(undefined);
+      }


cjcenizal · 2017-01-26T17:23:10Z

src/ui/public/modals/__tests__/confirm_modal.js

+        confirmModal('hi', { customConfirmButton: 'b', onConfirm: _.noop, showClose: true });
+        expect(true).to.be(false);
+      } catch (error) {
+        expect(error).to.not.be(undefined);


cjcenizal · 2017-01-26T17:24:06Z

src/ui/public/modals/__tests__/confirm_modal.js

+  it('shows custom text', function () {
+    const confirmModalOptions = {
+      confirmButtonText: 'Troodon',
+      cancelButtonText: 'Dilophosaurus',


More of an Allosaurus guy, myself.

cjcenizal · 2017-01-26T17:26:43Z

src/ui/public/modals/__tests__/confirm_modal.js

+    expect(cancelButtonText).to.equal('Dilophosaurus');
+    const titleText = findByDataTestSubj('confirmModalTitleText')[0].innerText;
+    expect(titleText).to.equal('Dinosaurs');
+  });


This is a bit dogmatic, but I think it would be better to make this a describe block with individual tests per each individual customizable unit:

describe('shows custom text', () => { it('for confirm button', () => { }); it('for cancel button', () => { }); it('for title text', () => { }); });

I think this documents the interface a bit better, and it also makes it easier to zoom in on failing tests and know exactly what's failing based on the error messages in the terminal.

cjcenizal · 2017-01-26T17:28:04Z

src/ui/public/modals/confirm_modal.html

@@ -1,4 +1,16 @@
 <div class="kuiModal" style="width: 450px" data-test-subj="confirmModal">
+  <div class="kuiModalHeader" ng-if="title">


Do we need this ng-if here? It seems unexpected to hide the close button if no title is provided. I think we can remove it entirely and just show an empty title element if no title is provided.

I think it looks weird without a title and an empty header. It can't be that hard to come up with a title (Warning, Error, Info at the most basic), so I kinda like forcing people to add one, even if only for aesthetic reasons. If you disagree I can make it optional though.

Yeah, you're right. You changed my mind. Still, there's a weird conflict if someone tries to set showClose to true, but doesn't specify a title. Since the component doesn't support that, we should let the engineer know they're misusing the interface. How about throwing an error?

:) Already handled. Even have a test for it! 😸

it('when showClose is on but title is not given', function () { const options = { customConfirmButton: 'b', onConfirm: _.noop, showClose: true }; expect(confirmModal('hi', options)).to.throwError(); });

cjcenizal · 2017-01-26T17:30:24Z

src/ui/public/modals/__tests__/confirm_modal.js

+      expect(xIcon.length).to.be(0);
+    });
+  });
+});


Can we add tests verifying that the onClose, onConfirm, and onCancel callbacks are executed when buttons are pressed?

++ Good idea

cjcenizal · 2017-01-26T17:36:17Z

src/ui/public/modals/confirm_modal.js

@@ -23,9 +27,15 @@ module.factory('confirmModal', function ($rootScope, $compile) {
  return function confirmModal(message, customOptions) {
    const defaultOptions = {
      onCancel: noop,
-      cancelButtonText: 'Cancel'
+      cancelButtonText: 'Cancel',
+      onClose: noop,


I think Stacey's suggestion is the best option for now.

Longer term, I don't think ConfirmModal is the right tool for this use case. I think we should only use ConfirmModal for yes/no questions and create different modals for situations like this one.

- Clean up tests and add some new ones - Default onClose to onCancel if nothing is supplied.

w33ble · 2017-01-26T19:42:53Z

src/ui/public/modals/__tests__/confirm_modal.js

+      confirmButtonText: 'bye',
+      onConfirm: confirmCallback,
+      onCancel: cancelCallback,
+      onClose: closeCallback,


I still feel very strongly that there should not be an onClose action on a confirmation dialog. Confirmation is yes/no, and closing the modal should do the same thing as clicking on cancel/no. If you need a third action, I think there should be a new type of modal.

Since my other comment got collapsed, see: #10069 (comment)

I'm giving the user the option to not answer the confirmation. It's like, Yes/No/Prefer not to answer right now.

I understand that's what you are doing, but that's not how confirm dialogs work. Look to the thing you are replacing, window.confirm(), as an example.

Adding a third option is confusing, both to the user and to the person implementing the confirm dialog. Personally when I see a confirmation dialog with a close button, I expect that closing the box is the same as answering no.

Now, if you would like to add a new kind of dialog/modal, where you can defer your answer until later and close the prompt for now, that's fine. But I don't think that confirmModel should work this way.

What would you propose instead? I can rename confirm => custom and rename all the buttons from cancel => secondary action and confirm => primary action?

I don't want to introduce a slew of extremely similar code just so I can can pass a different onClose than onCancel action, and that is the only difference between the modal that I need and the confirmModal that you would prefer.

Would you be content if I opened an issue for to explore this later? It feels like diminishing returns at this point. IMO it is not far fetched to allow a confirmation modal a custom onClose as a specialized option (not the default). Just because the native confirmation window works like that doesn't mean this has to. We allow them to set the cancel button text, so the user might not be answering 'no' or 'cancel', but something else.

Other "Confirmation" dialogs offer more than just yes/no as well:

Bootstrap custom confirm:

And jquery-confirm also doesn't make the close function match the onCancel function (at least I think that's how it works by reading the functions - looks like close just closes without calling onCancel) http://craftpip.github.io/jquery-confirm/v1.7.8/

I'm fine with improving this at some point, but I don't think it's the highest priority. Seems like something we could tackle when we address how to handle even more customized modals (e.g. passing in templates, etc). So unless there is a quick-ish solution that doesn't cause a bunch of duplicate code, I'd prefer to punt.

I think we can punt on this for now as Stacey suggests. Can we merge this PR and have an in-depth discussion about this later?

I personally don't think we should look to window.confirm as a reference. I think of what we're building as a modal system first and foremost. Beyond that, we're creating customized modals to support different UXes, of which confirmation modals are one instance. So while I agree that this is a hacky use of the confirmation modal, I think that the onClose functionality should be kept, and we can build a more appropriate, custom modal for this use-case later.

@w33ble Can you walk me through the thought-process and pain points for an engineer who has the ability to customize onClose? It seems ergonomic to me, since it defaults to the onCancel callback, which sounds like the behavior you're asking for. Maybe I'm overlooking something, though, so if this is a 10 on a pain scale of 1-10, then I want to be able to empathize.

Yeah, we can punt on it for now. I think it's enough that onClose defaults to onCancel, since it doesn't change the way this dialog works, and it should no longer trip up a developer trying to use it.

@stacey-gammon can you open a new issue and link back to this conversation?

Related: how do we handle keyboard input, like the escape key? #10099

Filed #10104. ++ on the issue you filed above, no handling of esc key right now.

w33ble

LGTM. Please open a new issue about the onClose handler so we can discuss our options somewhere else, then this is good to go.

Backports PR #10069 **Commit 1:** Add ability to pass a title and show the close button on a modal * Original sha: b66a900 * Authored by Stacey Gammon <[email protected]> on 2017-01-25T18:50:06Z **Commit 2:** address code review comments - Clean up tests and add some new ones - Default onClose to onCancel if nothing is supplied. * Original sha: 63e90f7 * Authored by Stacey Gammon <[email protected]> on 2017-01-26T18:01:58Z

) Backports PR #10069 **Commit 1:** Add ability to pass a title and show the close button on a modal * Original sha: b66a900 * Authored by Stacey Gammon <[email protected]> on 2017-01-25T18:50:06Z **Commit 2:** address code review comments - Clean up tests and add some new ones - Default onClose to onCancel if nothing is supplied. * Original sha: 63e90f7 * Authored by Stacey Gammon <[email protected]> on 2017-01-26T18:01:58Z

The reason they were added is no longer necessary (something for view edit mode which didn’t pan out). It was a point of contention: See elastic#10104 and elastic#10069 (review) for background. Since it’s no longer needed anyway, and this is going to get moved to react anyhow, I’m removing the unused functionality.

The reason they were added is no longer necessary (something for view edit mode which didn’t pan out). It was a point of contention: See #10104 and #10069 (review) for background. Since it’s no longer needed anyway, and this is going to get moved to react anyhow, I’m removing the unused functionality.

The reason they were added is no longer necessary (something for view edit mode which didn’t pan out). It was a point of contention: See elastic#10104 and elastic#10069 (review) for background. Since it’s no longer needed anyway, and this is going to get moved to react anyhow, I’m removing the unused functionality.

The reason they were added is no longer necessary (something for view edit mode which didn’t pan out). It was a point of contention: See #10104 and #10069 (review) for background. Since it’s no longer needed anyway, and this is going to get moved to react anyhow, I’m removing the unused functionality.

Add ability to pass a title and show the close button on a modal

b66a900

stacey-gammon requested review from cjcenizal and w33ble January 25, 2017 19:18

stacey-gammon added review v5.3.0 labels Jan 25, 2017

w33ble suggested changes Jan 25, 2017

View reviewed changes

w33ble self-assigned this Jan 25, 2017

cjcenizal suggested changes Jan 26, 2017

View reviewed changes

address code review comments

63e90f7

- Clean up tests and add some new ones - Default onClose to onCancel if nothing is supplied.

w33ble suggested changes Jan 26, 2017

View reviewed changes

w33ble approved these changes Jan 27, 2017

View reviewed changes

stacey-gammon mentioned this pull request Jan 28, 2017

Expand and clean up custom modal service #10104

Closed

stacey-gammon merged commit 58b5cbd into elastic:master Jan 28, 2017

elastic-jasper mentioned this pull request Jan 28, 2017

[5.x] Add ability to pass a title and show the close button on a modal #10105

Merged

stacey-gammon deleted the add-title-and-close-confirm branch January 30, 2017 15:39

stacey-gammon mentioned this pull request Apr 19, 2017

Remove showClose and onClose options in confirm modal #11321

Merged

stacey-gammon mentioned this pull request Apr 19, 2017

[5.x] Remove showClose and onClose options in confirm modal (#11321) #11334

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to pass a title and show the close button on a modal #10069

Add ability to pass a title and show the close button on a modal #10069

stacey-gammon commented Jan 25, 2017

w33ble Jan 25, 2017

stacey-gammon Jan 26, 2017

stacey-gammon Jan 26, 2017

cjcenizal Jan 26, 2017

w33ble Jan 26, 2017

cjcenizal left a comment

cjcenizal Jan 26, 2017

stacey-gammon Jan 26, 2017

cjcenizal Jan 26, 2017

cjcenizal Jan 26, 2017

cjcenizal Jan 26, 2017

cjcenizal Jan 26, 2017

cjcenizal Jan 26, 2017

stacey-gammon Jan 26, 2017

cjcenizal Jan 26, 2017 •

edited

Loading

stacey-gammon Jan 26, 2017

cjcenizal Jan 26, 2017

cjcenizal Jan 26, 2017

stacey-gammon Jan 26, 2017

cjcenizal Jan 26, 2017

w33ble Jan 26, 2017 •

edited

Loading

stacey-gammon Jan 26, 2017

w33ble Jan 27, 2017

stacey-gammon Jan 27, 2017 •

edited

Loading

cjcenizal Jan 27, 2017 •

edited

Loading

w33ble Jan 27, 2017 •

edited

Loading

w33ble Jan 27, 2017

stacey-gammon Jan 28, 2017

w33ble left a comment

		@@ -1,4 +1,16 @@
		<div class="kuiModal" style="width: 450px" data-test-subj="confirmModal">
		<div class="kuiModalHeader" ng-if="title">

Add ability to pass a title and show the close button on a modal #10069

Add ability to pass a title and show the close button on a modal #10069

Conversation

stacey-gammon commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

cjcenizal Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

w33ble Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble left a comment

Choose a reason for hiding this comment

cjcenizal Jan 26, 2017 •

edited

Loading

w33ble Jan 26, 2017 •

edited

Loading

stacey-gammon Jan 27, 2017 •

edited

Loading

cjcenizal Jan 27, 2017 •

edited

Loading

w33ble Jan 27, 2017 •

edited

Loading