replace KuiConfirmModal with EuiConfirmModal in confirm-modal directive #15693

nreese · 2017-12-19T17:24:26Z

replace KuiConfirmModal with EuiConfirmModal in confirm-modal directive

EuiConfirmModal is a drop in replacement for KuiConfirmModal but provides better styling - like theming for dark mode

cjcenizal

LGTM!

stacey-gammon · 2017-12-20T19:12:11Z

Should we give it a title? Or update the EUI one to not have so much space at the top if no title was given? Ultimately I'll defer to @cjcenizal, just wanted to point it out.

before:

after:

Otherwise, LGTM.

cjcenizal · 2017-12-20T19:52:41Z

@stacey-gammon Good catch! Yes, I think we should have a title, to adhere to the pattern in the EUI docs:

This will entail updating the 11 or so instances where we invoke confirmModal to make sure we're always providing it with a title.

cjcenizal · 2017-12-20T19:53:21Z

I'd suggest pairing up with @gchaps when figuring out what the titles should be.

gchaps · 2017-12-20T20:06:43Z

Yep, I can help. I'm not a fan of "Are you sure," though. Just extra words to read and not as direct.

nreese · 2017-12-20T20:48:26Z

The confirm modal is used in the following places. Titles will be needed for these.

Dashboard Editing - Cancel changes. Message text Are you sure you want to cancel and lose changes?
Deleting dashboards Are you sure you want to delete the selected dashboards? This action is irreversible!
Index Pattern management - refresh field list This will reset the field popularity counters. Are you sure you want to refresh your fields?
Index Pattern management - delete index pattern Are you sure you want to remove this index pattern?
Index Pattern management - delete scripted field Are you sure want to delete ${field.name}? This action is irreversible!
Index Pattern management - delete Source Filters Are you sure want to delete this filter?
Saved Object management - bulk delete Are you sure you want to delete the selected ${$scope.currentTab.title}? This action is irreversible!
Saved Object management - import If any of the objects already exist, do you want to automatically overwrite them?
Saved Object management - delete Are you sure want to delete this object? This action is irreversible!
Visualize - delete Are you sure you want to delete the selected visualizations? This action is irreversible!
Timelion - delete sheet Are you sure you want to delete the sheet ${title}?
Field editor - delete Are you sure want to delete '${self.field.name}'? This action is irreversible!

gchaps · 2017-12-21T20:35:32Z

gchaps · 2017-12-21T20:36:14Z

gchaps · 2017-12-21T20:37:17Z

gchaps · 2017-12-21T20:38:29Z

@nreese and I worked on the titles and body text.

nreese · 2018-01-02T18:10:49Z

@gchaps I think we need to update the text for 8, Import saved Kibana objects. The prompt is asking about auto updating all or getting a confirm button for each existing saved object before overwriting. How about something like the below screen shot?

nreese · 2018-01-02T19:20:43Z

@alexfrancoeur @elastic/kibana-design The existing implementation for the "Cancel dashboard editing" confirm modal contains logic to display the name(s) of any filters that have been edited.

export function getUnsavedChangesWarningMessage(changedFilters) {
  const changedFilterList = createStringList(changedFilters);

  return changedFilterList ?
    `Are you sure you want to cancel and lose changes, including changes made to your ${changedFilterList}?` :
    `Are you sure you want to cancel and lose changes?`;
}

The updated wording keeps things simple and just displays "Once you discard your changes, there's no getting them back". Number 1 in #15693 (comment).

Should the confirm message retain the specific warning about filters? Or should a static message be displayed?

cc @stacey-gammon

stacey-gammon · 2018-01-02T19:27:14Z

FYI, for the one about discarding changes, we have some additional logic that specifically points out changes made to your query an/or the time range (if time is saved with a dashboard).

We added this logic because these settings can be modified in view mode. If you then go into edit mode and hit cancel without changing anything, you'll get the warning but might be confused as to why. Here is an example flow:

It does the same thing if you edited the query in the query bar.

Personally I'm okay with getting rid of the logic and just using the simpler error message. I doubt people read that far into the message anyway. But I did want to point it out. @cjcenizal - do you have any thoughts on this, since you were a part of the original discussions for how to handle that odd situation of editing a part of a dashboard in view mode then going into edit mode?

gchaps · 2018-01-02T20:28:13Z

@nreese The title and the sentence below it say the same thing. Can we simply say:

Automatically overwrite all saved objects?

No, prompt for each object Yes, overwrite all objects

--or--

Automatically overwrite all saved objects?

Currently saved objects will be lost.

No, prompt for each object Yes, overwrite all objects

cjcenizal · 2018-01-03T19:55:43Z

Personally I'm okay with getting rid of the logic and just using the simpler error message

SGTM

nreese · 2018-01-03T22:01:49Z

@stacey-gammon @cjcenizal Would you mind taking another look at this PR. With the addition of rewording each confirmation modal, lots has changed since your initial approvals.

cjcenizal

Changes LGTM overall, just had some questions for Gail.

cjcenizal · 2018-01-05T22:35:20Z

...anagement/sections/indices/edit_index_pattern/scripted_fields_table/scripted_fields_table.js

-            onConfirm: () => { $scope.indexPattern.removeScriptedField(field.name); }
+            confirmButtonText: 'Delete',
+            onConfirm: () => { $scope.indexPattern.removeScriptedField(field.name); },
+            title: `Delete scripted field ${field.name}?`


@gchaps Should we wrap custom names of things in quotation marks? For example, if the scripted field happens to be named "field", this message will read:

Delete scripted field field?

It seems like any generic-sounding word will make that sentence read funny. I think quotation marks would clarify the question:

Delete scripted field "field"?

cjcenizal · 2018-01-05T22:35:57Z

src/core_plugins/timelion/public/app.js

@@ -117,9 +117,10 @@ app.controller('timelion', function (

      const confirmModalOptions = {
        onConfirm: doDelete,
-        confirmButtonText: 'Delete sheet'
+        confirmButtonText: 'Delete',
+        title: `Delete Timelion sheet ${title}?`


@gchaps Same idea here regarding quotation marks.

cjcenizal · 2018-01-05T22:36:04Z

src/ui/public/field_editor/field_editor.js

-            onConfirm: doDelete
+            confirmButtonText: 'Delete',
+            onConfirm: doDelete,
+            title: `Delete field ${self.field.name}?`


@gchaps Same idea here regarding quotation marks.

Good point, @cjcenizal

Our writing guidelines say to avoid unnecessary punctuation unless it helps clarify meaning, which in this case it does. I did some research and I noticed that others do use quotation marks around a field name in a title. For example:

Delete file “New Text Document”

If this looks too heavy, we can also try single quotation marks:

Delete file ‘New Text Document’

Or if you don’t want punctuation in the title, we can use a title followed by a sentence. For example,

Delete this file?

The file ‘New Text Document’ will be moved to the Recycle bin.

My preference is to use single quotation marks.

Single quotation marks sounds good to me. What do you think @nreese ?

@gchaps Can you also update the writing guidelines with this rule?

@cjcenizal Yes, good idea to add this to the guidelines.

… clean up wording for import saved objects modal

…ve (elastic#15693) * replace KuiConfirmModal with EuiConfirmModal * add titles to confirm modals. Update modal text and button text * updates for newest version of EUI * use EUI class in overlay and EUI button constants * remove logic about changed filters in cancel dashboard editing modal, clean up wording for import saved objects modal * fix broken function test - management delete index pattern * wrap names in single quotes

…ve (#15693) (#15898) * replace KuiConfirmModal with EuiConfirmModal * add titles to confirm modals. Update modal text and button text * updates for newest version of EUI * use EUI class in overlay and EUI button constants * remove logic about changed filters in cancel dashboard editing modal, clean up wording for import saved objects modal * fix broken function test - management delete index pattern * wrap names in single quotes

nreese requested review from cjcenizal and stacey-gammon December 19, 2017 17:24

cjcenizal approved these changes Dec 19, 2017

View reviewed changes

cjcenizal added Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. v6.2.0 v7.0.0 :Sharing and removed Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. labels Dec 19, 2017

stacey-gammon approved these changes Dec 20, 2017

View reviewed changes

chrisronline mentioned this pull request Dec 20, 2017

[META] EUIfication #15721

Closed

47 tasks

nreese mentioned this pull request Dec 21, 2017

update DashboardCloneModal to use EUI framework #15697

Merged

nreese force-pushed the confirmModal branch from 85a5b1f to 8bdf053 Compare January 2, 2018 16:14

nreese force-pushed the confirmModal branch from 17ccfdf to 15cc940 Compare January 2, 2018 22:29

cjcenizal reviewed Jan 5, 2018

View reviewed changes

replace KuiConfirmModal with EuiConfirmModal

a923c79

nreese added 5 commits January 8, 2018 08:20

add titles to confirm modals. Update modal text and button text

41895c3

updates for newest version of EUI

58a302f

use EUI class in overlay and EUI button constants

94e2e66

remove logic about changed filters in cancel dashboard editing modal,…

04b9906

… clean up wording for import saved objects modal

fix broken function test - management delete index pattern

ec81aeb

nreese force-pushed the confirmModal branch from c7e64dd to ec81aeb Compare January 8, 2018 15:21

wrap names in single quotes

c752220

nreese merged commit faf0633 into elastic:master Jan 8, 2018

nreese mentioned this pull request Jan 8, 2018

[6.x] replace KuiConfirmModal with EuiConfirmModal in confirm-modal directive (#15693) #15898

Merged

nreese mentioned this pull request Jan 21, 2018

Missing warning text when deleting a visualization #15633

Closed

nreese deleted the confirmModal branch February 13, 2018 19:31

aphelionz mentioned this pull request Jun 12, 2018

Accessibility: Kibana 6.3 Meta Issue #18866

Closed

48 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace KuiConfirmModal with EuiConfirmModal in confirm-modal directive #15693

replace KuiConfirmModal with EuiConfirmModal in confirm-modal directive #15693

nreese commented Dec 19, 2017 •

edited

Loading

cjcenizal left a comment

stacey-gammon commented Dec 20, 2017 •

edited

Loading

cjcenizal commented Dec 20, 2017

cjcenizal commented Dec 20, 2017

gchaps commented Dec 20, 2017

nreese commented Dec 20, 2017

gchaps commented Dec 21, 2017

gchaps commented Dec 21, 2017

gchaps commented Dec 21, 2017

gchaps commented Dec 21, 2017

nreese commented Jan 2, 2018

nreese commented Jan 2, 2018

stacey-gammon commented Jan 2, 2018

gchaps commented Jan 2, 2018

cjcenizal commented Jan 3, 2018

nreese commented Jan 3, 2018 •

edited

Loading

cjcenizal left a comment •

edited

Loading

cjcenizal Jan 5, 2018

cjcenizal Jan 5, 2018

cjcenizal Jan 5, 2018

gchaps Jan 5, 2018

cjcenizal Jan 5, 2018 •

edited

Loading

cjcenizal Jan 5, 2018

gchaps Jan 6, 2018

replace KuiConfirmModal with EuiConfirmModal in confirm-modal directive #15693

replace KuiConfirmModal with EuiConfirmModal in confirm-modal directive #15693

Conversation

nreese commented Dec 19, 2017 • edited Loading

cjcenizal left a comment

Choose a reason for hiding this comment

stacey-gammon commented Dec 20, 2017 • edited Loading

cjcenizal commented Dec 20, 2017

cjcenizal commented Dec 20, 2017

gchaps commented Dec 20, 2017

nreese commented Dec 20, 2017

gchaps commented Dec 21, 2017

gchaps commented Dec 21, 2017

gchaps commented Dec 21, 2017

gchaps commented Dec 21, 2017

nreese commented Jan 2, 2018

nreese commented Jan 2, 2018

stacey-gammon commented Jan 2, 2018

gchaps commented Jan 2, 2018

cjcenizal commented Jan 3, 2018

nreese commented Jan 3, 2018 • edited Loading

cjcenizal left a comment • edited Loading

Choose a reason for hiding this comment

cjcenizal Jan 5, 2018

Choose a reason for hiding this comment

cjcenizal Jan 5, 2018

Choose a reason for hiding this comment

cjcenizal Jan 5, 2018

Choose a reason for hiding this comment

gchaps Jan 5, 2018

Choose a reason for hiding this comment

cjcenizal Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

cjcenizal Jan 5, 2018

Choose a reason for hiding this comment

gchaps Jan 6, 2018

Choose a reason for hiding this comment

nreese commented Dec 19, 2017 •

edited

Loading

stacey-gammon commented Dec 20, 2017 •

edited

Loading

nreese commented Jan 3, 2018 •

edited

Loading

cjcenizal left a comment •

edited

Loading

cjcenizal Jan 5, 2018 •

edited

Loading