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

Introduce custom modal confirmation dialogs #9859

Merged
merged 10 commits into from Jan 19, 2017

Conversation

Projects
None yet
4 participants
@Stacey-Gammon
Contributor

Stacey-Gammon commented Jan 13, 2017

Stop using native confirm dialogs and create a custom one. @cjcenizal is planning on further improving the UI of the dialog but here it is for now:

screen shot 2017-01-12 at 4 52 30 pm

You can test it out in timelion by trying to delete a saved sheet. All usages of safeConfirm will now use this custom modal dialog.

@cjcenizal

Sweeeet! I had a lot of really small suggestions, and a couple larger ones that were centered on the UI.

My suggestions about changing the modal markup and styles will result in the modal looking like this:

image

@Stacey-Gammon

This comment has been minimized.

Show comment
Hide comment
@Stacey-Gammon

Stacey-Gammon Jan 13, 2017

Contributor

@cjcenizal Addressed most of your requested changes, and also tried to implement the new style guide after discussing some stuff with @w33ble and he had some good other suggestions. Please take another look and let me know what you think!

Contributor

Stacey-Gammon commented Jan 13, 2017

@cjcenizal Addressed most of your requested changes, and also tried to implement the new style guide after discussing some stuff with @w33ble and he had some good other suggestions. Please take another look and let me know what you think!

@cjcenizal

Joe and I talked about the most recent changes to the PR and we came to a couple conclusions:

  1. Logic is best extracted out of an Angular service into a vanilla JS service when the resulting JS service has no Angular dependencies. This makes the service more reusable, testable, and easier to migrate into a non-Angular codebase.
  2. Extracting logic into a vanilla JS service creates indirection and adds complexity, which is only a good tradeoff if the above criteria can be met.
  3. The file modal_dialogue.js is more of a "teleport" for adding and removing an element to the body, and its knowledge of scope dilutes this single responsibility.

So, I think we should revert to the most recent commit, but preserve the changes made in response to my comments, as well as preserve the single point of entry in modules.js.

Let's Zoom about this some more to make sure this makes sense. We had a long discussion and I may not be completely representing it here.

Joe's working on a POC to explore some other ideas he had as we were talking, partially inspired by the Modal demo I showed you (http://smaato.github.io/ui-framework/#/modal).

@tbragin tbragin added the :Design label Jan 14, 2017

@Stacey-Gammon

This comment has been minimized.

Show comment
Hide comment
@Stacey-Gammon

Stacey-Gammon Jan 16, 2017

Contributor

Are the new rules/suggestions using this .factory style/wrapper written down anywhere? If not I think we should keep track of the questions that arise while we experiment with it, and keep track of the answers decided on. For instance don't extricate vanilla js if it's tightly bound to angular.

After the evolving codebase email that went around at the end of last week, I think the sooner we can embrace this team wide, the better. I was thinking about that part of the article @Bargs shared If you don't know where you're going, you're lost and incrementalism is just a delusion to trick you into thinking you're making meaningful progress.. If we don't have team wide adoption of this style then I'm just introducing further disparities into the code base - directly going against what @Bargs said in that email thread "for existing plugins/apps consistency really does trump everything else".

Also @cjcenizal you had said in that email a new pattern's success depends on two criteria: a) understanding and embrace by the team, and b) consistent application by the team throughout the codebase.

I don't think we've hit a) yet with this new pattern. I like it a lot, but after thinking about that email, I'm not sure I should be adopting it at this point. I guess I don't see the difference between me introducing this new pattern/style and what @weltenwort was trying to do. Both are attempts at creating new patterns in the code base but it looks like @weltenwort was held to the consistency/team adoption standard, while this pattern is being allowed to slip by. Perhaps this is my fault since I am the first person to introduce it to the existing code base, I'm just having second thoughts after that email and wondering if adopting the .factory.js files at this point is the right move.

Contributor

Stacey-Gammon commented Jan 16, 2017

Are the new rules/suggestions using this .factory style/wrapper written down anywhere? If not I think we should keep track of the questions that arise while we experiment with it, and keep track of the answers decided on. For instance don't extricate vanilla js if it's tightly bound to angular.

After the evolving codebase email that went around at the end of last week, I think the sooner we can embrace this team wide, the better. I was thinking about that part of the article @Bargs shared If you don't know where you're going, you're lost and incrementalism is just a delusion to trick you into thinking you're making meaningful progress.. If we don't have team wide adoption of this style then I'm just introducing further disparities into the code base - directly going against what @Bargs said in that email thread "for existing plugins/apps consistency really does trump everything else".

Also @cjcenizal you had said in that email a new pattern's success depends on two criteria: a) understanding and embrace by the team, and b) consistent application by the team throughout the codebase.

I don't think we've hit a) yet with this new pattern. I like it a lot, but after thinking about that email, I'm not sure I should be adopting it at this point. I guess I don't see the difference between me introducing this new pattern/style and what @weltenwort was trying to do. Both are attempts at creating new patterns in the code base but it looks like @weltenwort was held to the consistency/team adoption standard, while this pattern is being allowed to slip by. Perhaps this is my fault since I am the first person to introduce it to the existing code base, I'm just having second thoughts after that email and wondering if adopting the .factory.js files at this point is the right move.

@Stacey-Gammon

This comment has been minimized.

Show comment
Hide comment
@Stacey-Gammon

Stacey-Gammon Jan 16, 2017

Contributor

I merged @w33ble's PR and made a few small adjustments like moving safe_confirm into the modals folder.

I have a question though - why the change to pass onConfirm and onCancel instead of returning a promise? Also making them default to noop feels a bit weird to me. If the default is to do nothing, there really shouldn't be two buttons. I could see making one optional, but not both. But then if you make one optional, which one?

Contributor

Stacey-Gammon commented Jan 16, 2017

I merged @w33ble's PR and made a few small adjustments like moving safe_confirm into the modals folder.

I have a question though - why the change to pass onConfirm and onCancel instead of returning a promise? Also making them default to noop feels a bit weird to me. If the default is to do nothing, there really shouldn't be two buttons. I could see making one optional, but not both. But then if you make one optional, which one?

Stacey-Gammon and others added some commits Jan 13, 2017

address code review comments
and implement new angular style suggestions from Joe
refactor
no point making vanilla code that's really only going to work with angular i guess...

also broke apart the confirm modal from the injector, and made safeConfirm just use the confirm modal
Clean up from merge
 - Move safe_confirm into modals folder
 - make data-test-subj camelCase.
 - get rid of $timeout call, doesn’t seem necessary anymore, now that
window.confirm is not being used.
@w33ble

This comment has been minimized.

Show comment
Hide comment
@w33ble

w33ble Jan 17, 2017

Member

Are the new rules/suggestions using this .factory style/wrapper written down anywhere? If not I think we should keep track of the questions that arise while we experiment with it, and keep track of the answers decided on. For instance don't extricate vanilla js if it's tightly bound to angular.

This is all still very much in flux, it's just a pattern we've been happy with elsewhere. I still would have liked to break out the vanilla code, even just to make testing easier, but I see CJ's point in that the code is useless without Angular. But mocking the Angular stuff isn't that hard either, so I'm a little torn. What do you think?

If we don't have team wide adoption of this style then I'm just introducing further disparities into the code base - directly going against what @Bargs said in that email thread "for existing plugins/apps consistency really does trump everything else".

I agree with what @Bargs has said. I didn't know of anyone else proposing other patterns at the moment though, and still need to dive in to what @weltenwort has been doing. Planning to sync with him this week. Like I said, the management team has been pretty happy with these patterns, but we do need to get larger buy-in from the team, and I think it's important to get feedback from more than just the three of us; that's a big part of why I brought this up with you originally. We're still very much open to other ideas.

If you decide not to follow our patterns here, it's really up to you. This is your PR after all.

I have a question though - why the change to pass onConfirm and onCancel instead of returning a promise?

I though it best to let the click handlers decide what to do when the user clicks the button. This way, the modal is only concerned with executing some handler and closing itself. We may not always want to deal with a promise, and it has the added bonus of one less dependency within confirm_modal.

If you disagree, feel free to change it up.

Member

w33ble commented Jan 17, 2017

Are the new rules/suggestions using this .factory style/wrapper written down anywhere? If not I think we should keep track of the questions that arise while we experiment with it, and keep track of the answers decided on. For instance don't extricate vanilla js if it's tightly bound to angular.

This is all still very much in flux, it's just a pattern we've been happy with elsewhere. I still would have liked to break out the vanilla code, even just to make testing easier, but I see CJ's point in that the code is useless without Angular. But mocking the Angular stuff isn't that hard either, so I'm a little torn. What do you think?

If we don't have team wide adoption of this style then I'm just introducing further disparities into the code base - directly going against what @Bargs said in that email thread "for existing plugins/apps consistency really does trump everything else".

I agree with what @Bargs has said. I didn't know of anyone else proposing other patterns at the moment though, and still need to dive in to what @weltenwort has been doing. Planning to sync with him this week. Like I said, the management team has been pretty happy with these patterns, but we do need to get larger buy-in from the team, and I think it's important to get feedback from more than just the three of us; that's a big part of why I brought this up with you originally. We're still very much open to other ideas.

If you decide not to follow our patterns here, it's really up to you. This is your PR after all.

I have a question though - why the change to pass onConfirm and onCancel instead of returning a promise?

I though it best to let the click handlers decide what to do when the user clicks the button. This way, the modal is only concerned with executing some handler and closing itself. We may not always want to deal with a promise, and it has the added bonus of one less dependency within confirm_modal.

If you disagree, feel free to change it up.

@Stacey-Gammon

This comment has been minimized.

Show comment
Hide comment
@Stacey-Gammon

Stacey-Gammon Jan 17, 2017

Contributor

@cjcenizal @w33ble thanks for the chat today, helped a lot. Code should be updated as discussed plus most recent comments.

Contributor

Stacey-Gammon commented Jan 17, 2017

@cjcenizal @w33ble thanks for the chat today, helped a lot. Code should be updated as discussed plus most recent comments.

@@ -0,0 +1,3 @@
import './confirm_modal';
import './safe_confirm';

This comment has been minimized.

@w33ble

w33ble Jan 17, 2017

Member

Having safe_confirm in modals seems weird. The safe_confirm is still its own service, the fact that is uses a modal now is an implementation detail...

@w33ble

w33ble Jan 17, 2017

Member

Having safe_confirm in modals seems weird. The safe_confirm is still its own service, the fact that is uses a modal now is an implementation detail...

This comment has been minimized.

@Stacey-Gammon

Stacey-Gammon Jan 18, 2017

Contributor

It was always a modal, it's just that now it uses our custom modal. Even if we left it using the native modal I think it could still go in a modals folder.

There is nothing else that went in the previous safe_confirm folder (except tests) so it doesn't deserve a dedicated extra folder IMO. Is there another location you think it could go that isn't in it's own dedicated folder?

@Stacey-Gammon

Stacey-Gammon Jan 18, 2017

Contributor

It was always a modal, it's just that now it uses our custom modal. Even if we left it using the native modal I think it could still go in a modals folder.

There is nothing else that went in the previous safe_confirm folder (except tests) so it doesn't deserve a dedicated extra folder IMO. Is there another location you think it could go that isn't in it's own dedicated folder?

@w33ble

This comment has been minimized.

Show comment
Hide comment
@w33ble

w33ble Jan 17, 2017

Member

Overall, this looks great!

One UX detail - it would be great if safeConfirm/confirmModal would focus the Okay/Confirm button by default, so users could just hit enter and confirm the dialog. This is how the vanilla window.confirm dialog works. Breaking that and requiring a click would be super annoying to anyone used to the default confirm dialog.

Member

w33ble commented Jan 17, 2017

Overall, this looks great!

One UX detail - it would be great if safeConfirm/confirmModal would focus the Okay/Confirm button by default, so users could just hit enter and confirm the dialog. This is how the vanilla window.confirm dialog works. Breaking that and requiring a click would be super annoying to anyone used to the default confirm dialog.

@Stacey-Gammon

This comment has been minimized.

Show comment
Hide comment
@Stacey-Gammon

Stacey-Gammon Jan 18, 2017

Contributor

@w33ble good point about default focus, fixed!

Contributor

Stacey-Gammon commented Jan 18, 2017

@w33ble good point about default focus, fixed!

@w33ble

This comment has been minimized.

Show comment
Hide comment
@w33ble

w33ble Jan 18, 2017

Member

Love this change. LGTM!

Member

w33ble commented Jan 18, 2017

Love this change. LGTM!

@cjcenizal

I have one stupid suggestion; then merge away!

Show outdated Hide outdated src/ui/public/modals/confirm_modal.html

@Stacey-Gammon Stacey-Gammon merged commit 6b009a1 into elastic:master Jan 19, 2017

2 checks passed

CLA Commit author has signed the CLA
Details
kibana-ci Build finished.
Details

elastic-jasper added a commit that referenced this pull request Jan 19, 2017

Introduce custom modal confirmation dialogs
Backports PR #9859

**Commit 1:**
Introduce custom modal confirmation dialogs

* Original sha: 6b727df
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-13T15:56:44Z

**Commit 2:**
address code review comments

and implement new angular style suggestions from Joe

* Original sha: 5ca36df
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-13T22:12:44Z

**Commit 3:**
refactor

no point making vanilla code that's really only going to work with angular i guess...

also broke apart the confirm modal from the injector, and made safeConfirm just use the confirm modal

* Original sha: ce783b2
* Authored by Joe Fleming <joe.fleming@gmail.com> on 2017-01-14T01:09:52Z
* Committed by Stacey Gammon <gammon@elastic.co> on 2017-01-17T14:30:21Z

**Commit 4:**
Clean up from merge

 - Move safe_confirm into modals folder
 - make data-test-subj camelCase.
 - get rid of $timeout call, doesn’t seem necessary anymore, now that
window.confirm is not being used.

* Original sha: c91fbc6
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-16T15:00:14Z

**Commit 5:**
Fix tests

* Original sha: e581374
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-16T15:42:41Z

**Commit 6:**
Use ui-framework styles

* Original sha: d1e2b12
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-17T15:22:52Z

**Commit 7:**
Address latest round of comments and remove adoption of new patterns.

* Original sha: 0b690f1
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-17T20:00:52Z

**Commit 8:**
Improve error message. Use sinon in tests.

* Original sha: 92f815d
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-17T20:07:22Z

**Commit 9:**
Focus on the confirm button by default

* Original sha: 27be4df
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-18T14:17:44Z

**Commit 10:**
Indent message

* Original sha: 3dc6c93
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-18T22:04:49Z

Stacey-Gammon added a commit that referenced this pull request Jan 19, 2017

Introduce custom modal confirmation dialogs (#9958)
Backports PR #9859

**Commit 1:**
Introduce custom modal confirmation dialogs

* Original sha: 6b727df
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-13T15:56:44Z

**Commit 2:**
address code review comments

and implement new angular style suggestions from Joe

* Original sha: 5ca36df
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-13T22:12:44Z

**Commit 3:**
refactor

no point making vanilla code that's really only going to work with angular i guess...

also broke apart the confirm modal from the injector, and made safeConfirm just use the confirm modal

* Original sha: ce783b2
* Authored by Joe Fleming <joe.fleming@gmail.com> on 2017-01-14T01:09:52Z
* Committed by Stacey Gammon <gammon@elastic.co> on 2017-01-17T14:30:21Z

**Commit 4:**
Clean up from merge

 - Move safe_confirm into modals folder
 - make data-test-subj camelCase.
 - get rid of $timeout call, doesn’t seem necessary anymore, now that
window.confirm is not being used.

* Original sha: c91fbc6
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-16T15:00:14Z

**Commit 5:**
Fix tests

* Original sha: e581374
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-16T15:42:41Z

**Commit 6:**
Use ui-framework styles

* Original sha: d1e2b12
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-17T15:22:52Z

**Commit 7:**
Address latest round of comments and remove adoption of new patterns.

* Original sha: 0b690f1
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-17T20:00:52Z

**Commit 8:**
Improve error message. Use sinon in tests.

* Original sha: 92f815d
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-17T20:07:22Z

**Commit 9:**
Focus on the confirm button by default

* Original sha: 27be4df
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-18T14:17:44Z

**Commit 10:**
Indent message

* Original sha: 3dc6c93
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-18T22:04:49Z

@Stacey-Gammon Stacey-Gammon deleted the Stacey-Gammon:modal-dialogs branch Apr 6, 2017

Dreadnoth added a commit to Dreadnoth/kibana that referenced this pull request Aug 8, 2017

Introduce custom modal confirmation dialogs (#9958)
Backports PR #9859

**Commit 1:**
Introduce custom modal confirmation dialogs

* Original sha: 6b727df
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-13T15:56:44Z

**Commit 2:**
address code review comments

and implement new angular style suggestions from Joe

* Original sha: 5ca36df
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-13T22:12:44Z

**Commit 3:**
refactor

no point making vanilla code that's really only going to work with angular i guess...

also broke apart the confirm modal from the injector, and made safeConfirm just use the confirm modal

* Original sha: ce783b2
* Authored by Joe Fleming <joe.fleming@gmail.com> on 2017-01-14T01:09:52Z
* Committed by Stacey Gammon <gammon@elastic.co> on 2017-01-17T14:30:21Z

**Commit 4:**
Clean up from merge

 - Move safe_confirm into modals folder
 - make data-test-subj camelCase.
 - get rid of $timeout call, doesn’t seem necessary anymore, now that
window.confirm is not being used.

* Original sha: c91fbc6
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-16T15:00:14Z

**Commit 5:**
Fix tests

* Original sha: e581374
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-16T15:42:41Z

**Commit 6:**
Use ui-framework styles

* Original sha: d1e2b12
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-17T15:22:52Z

**Commit 7:**
Address latest round of comments and remove adoption of new patterns.

* Original sha: 0b690f1
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-17T20:00:52Z

**Commit 8:**
Improve error message. Use sinon in tests.

* Original sha: 92f815d
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-17T20:07:22Z

**Commit 9:**
Focus on the confirm button by default

* Original sha: 27be4df
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-18T14:17:44Z

**Commit 10:**
Indent message

* Original sha: 3dc6c93
* Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-18T22:04:49Z
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment