Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 114 additions & 0 deletions src/ui/public/modals/__tests__/confirm_modal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import angular from 'angular';
import expect from 'expect.js';
import ngMock from 'ng_mock';
import _ from 'lodash';

describe('ui/modals/confirm_modal', function () {
let confirmModal;
let $rootScope;

beforeEach(function () {
ngMock.module('kibana');
ngMock.inject(function ($injector) {
confirmModal = $injector.get('confirmModal');
$rootScope = $injector.get('$rootScope');
});
});

function findByDataTestSubj(dataTestSubj) {
return angular.element(document.body).find(`[data-test-subj=${dataTestSubj}]`);
}

afterEach(function () {
const confirmButton = findByDataTestSubj('confirmModalConfirmButton');
if (confirmButton) {
angular.element(confirmButton).click();
}
});

describe('throws an exception', function () {
it('when no custom confirm button passed', function () {
try {
confirmModal('hi', { onConfirm: _.noop });
expect(true).to.be(false);
} catch (error) {
expect(error).to.not.be(undefined);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fancy! Now that looks much better.

});

it('when no custom noConfirm function is passed', function () {
try {
confirmModal('hi', { confirmButtonText: 'bye' });
expect(true).to.be(false);
} catch (error) {
expect(error).to.not.be(undefined);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

});

it('when showClose is on but title is not given', function () {
try {
confirmModal('hi', { customConfirmButton: 'b', onConfirm: _.noop, showClose: true });
expect(true).to.be(false);
} catch (error) {
expect(error).to.not.be(undefined);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

}
});
});

it('shows the message', function () {
const myMessage = 'Hi, how are you?';
confirmModal(myMessage, { confirmButtonText: 'GREAT!', onConfirm: _.noop });

$rootScope.$digest();
const message = findByDataTestSubj('confirmModalBodyText')[0].innerText;
expect(message).to.equal(myMessage);
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More of an Allosaurus guy, myself.

title: 'Dinosaurs',
onConfirm: _.noop
};
confirmModal('What\'s your favorite dinosaur?', confirmModalOptions);

$rootScope.$digest();
const confirmButtonText = findByDataTestSubj('confirmModalConfirmButton')[0].innerText;
expect(confirmButtonText).to.equal('Troodon');
const cancelButtonText = findByDataTestSubj('confirmModalCancelButton')[0].innerText;
expect(cancelButtonText).to.equal('Dilophosaurus');
const titleText = findByDataTestSubj('confirmModalTitleText')[0].innerText;
expect(titleText).to.equal('Dinosaurs');
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


describe('x icon', function () {
it('is visible when showClose is true', function () {
const confirmModalOptions = {
confirmButtonText: 'bye',
onConfirm: _.noop,
showClose: true,
title: 'hi'
};
confirmModal('hi', confirmModalOptions);

$rootScope.$digest();
const xIcon = findByDataTestSubj('confirmModalCloseButton');
expect(xIcon.length).to.be(1);
});

it('is not visible when showClose is false', function () {
const confirmModalOptions = {
confirmButtonText: 'bye',
onConfirm: _.noop,
title: 'hi',
showClose: false
};
confirmModal('hi', confirmModalOptions);

$rootScope.$digest();
const xIcon = findByDataTestSubj('confirmModalCloseButton');
expect(xIcon.length).to.be(0);
});
});
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ Good idea

12 changes: 12 additions & 0 deletions src/ui/public/modals/confirm_modal.html
Original file line number Diff line number Diff line change
@@ -1,4 +1,16 @@
<div class="kuiModal" style="width: 450px" data-test-subj="confirmModal">
<div class="kuiModalHeader" ng-if="title">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@cjcenizal cjcenizal Jan 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) 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();
    });

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!!

<div class="kuiModalHeader__title" data-test-subj="confirmModalTitleText">
{{title}}
</div>

<div
ng-if="showClose"
class="kuiModalHeaderCloseButton kuiIcon fa-times"
data-test-subj="confirmModalCloseButton"
ng-click="onClose()"
></div>
</div>
<div class="kuiModalBody">
<div
class="kuiModalBodyText"
Expand Down
18 changes: 17 additions & 1 deletion src/ui/public/modals/confirm_modal.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ const module = uiModules.get('kibana');
* @property {String=} cancelButtonText
* @property {function} onConfirm
* @property {function=} onCancel
* @property {String=} title - If given, shows a title on the confirm modal. A title must be given if
* showClose is true, for aesthetic reasons.
* @property {Boolean=} showClose - If true, shows an [x] icon close button which by default is a noop
* @property {function=} onClose - Custom close button to call if showClose is true
*/

module.factory('confirmModal', function ($rootScope, $compile) {
Expand All @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
screen shot 2017-01-17 at 4 29 50 pm

Here 'x' will cancel out leaving edit mode:
screen shot 2017-01-17 at 4 30 55 pm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

showClose: false
};

if (customOptions.showClose === true && !customOptions.title) {
throw new Error('A title must be supplied when a close icon is shown');
}

if (!customOptions.confirmButtonText || !customOptions.onConfirm) {
throw new Error('Please specify confirmation button text and onConfirm action');
}
Expand All @@ -41,6 +51,8 @@ module.factory('confirmModal', function ($rootScope, $compile) {
confirmScope.message = message;
confirmScope.confirmButtonText = options.confirmButtonText;
confirmScope.cancelButtonText = options.cancelButtonText;
confirmScope.title = options.title;
confirmScope.showClose = options.showClose;
confirmScope.onConfirm = () => {
destroy();
options.onConfirm();
Expand All @@ -49,6 +61,10 @@ module.factory('confirmModal', function ($rootScope, $compile) {
destroy();
options.onCancel();
};
confirmScope.onClose = () => {
destroy();
options.onClose();
};

const modalInstance = $compile(template)(confirmScope);
modalPopover = new ModalOverlay(modalInstance);
Expand Down