Index pattern field editor - Add warning on name or type change by mattkime · Pull Request #95528 · elastic/kibana

mattkime · 2021-03-26T14:48:31Z

Summary

Adds a warning when field name or type is changed.

Unit or functional tests were updated or added to match the most common scenarios

mattkime · 2021-03-26T15:48:36Z

@sebelga It seems that adding the onChange prop to the name field triggers validation when the form is initially opened. Do you know why this might be happening? Its also possible that there's a better way to solve the problem than what I'm doing here.

Screen.Recording.2021-03-25.at.6.36.04.PM.mov

mattkime · 2021-03-27T04:38:06Z

@elasticmachine merge upstream

mattkime · 2021-04-05T14:03:32Z

@elasticmachine merge upstream

sebelga · 2021-04-05T14:40:42Z

src/plugins/index_pattern_field_editor/public/components/field_editor/field_editor.tsx

  },
 });

+const changeWarning = i18n.translate('indexPatternFieldEditor.editor.form.changeWarning', {


nit: this might be better inside the geti18nTexts handler above.

The geti18nTexts returns { title: string; description: JSX.Element | string }; and this is a single string.

Oh I missed that!

src/plugins/index_pattern_field_editor/public/components/field_editor/field_editor.tsx

elasticmachine · 2021-04-05T19:40:52Z

Pinging @elastic/kibana-app-services (Team:AppServices)

sebelga · 2021-04-06T15:56:06Z

@elasticmachine merge upstream

src/plugins/index_pattern_field_editor/public/components/field_editor/field_editor.tsx

…_editor/field_editor.tsx Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>

sebelga

LGTM! Thanks for adding this @mattkime .

As I put in the comment, I think it would be better to have a warning callout instead of a form error message. As this is not a form error but a caution for the user.

sebelga · 2021-04-07T10:59:43Z

src/plugins/index_pattern_field_editor/public/components/field_editor/field_editor.tsx

      </EuiFlexGroup>

+      {(nameHasChanged || typeHasChanged) && (
+        <EuiFormErrorText data-test-subj="changeWarning">{changeWarning}</EuiFormErrorText>


I wonder if a callout warning wouldn't be more appropriate?

{(nameHasChanged || typeHasChanged) && ( <> <EuiSpacer size="xs" /> <EuiCallOut color="warning" title="Name/type change" iconType="alert" size="s"> {changeWarning} </EuiCallOut> </> )}

If we go that path we can then have the translation (title and description) as part of the geti18nTexts() handler.

I'm curious what @ryankeairns thinks - I agree that its not the best to conflate designs for errors and warnings, but its also odd to have a warning thats larger than an error.

@sebelga @ryankeairns is out on vacation for a week. I think we should move forward with this and circle back later.

Maybe @mdefazio could give us his opinion? Basically between the warning message in the screenshot here and the one in the PR description. Thanks!

Here's some quick thoughts:

Can we append (or prepend) the warning icon to the form field if it's changed? and then show that message on hover or click?

Maybe just use the title in the callout instead of title+description. This will shrink the height of the callout and maybe find a middle ground between our concerns.

Perhaps a simpler option would be to show a confirmation dialog with this message when saving. If we do option 1, I think we would still need this approach too.

Happy to work through some more options as well as these are just some that come to mind.

To me, the warning callout is too loud. We follow it up with confirm dialog with the same content. Do we really need both? I'm afraid that if we're too loud people might think they're doing something unwise.

I really don't like just showing the title of the warning as then we're failing to explain why we're showing it.

I don't want to block on this, just sharing the pattern we've been applying in almost all of our forms. When there is something the user needs to be aware of in a form we either user the yellow or the blue callout. Even if it might look "big", if we consider that it is an important information for him to know we use the EuiCallout as they are meant for that.

If we consider it is not that important in the form and prefer just to have to confirm modal, that could be enough. But personally I don't mind having both to inform the user as soon as possible.

But again, happy if we go either way and don't want to block this PR on this. 😊

I really don't like just showing the title of the warning as then we're failing to explain why we're showing it.

I agree with this, and to be clearer, my thought was putting Changing name or type... into the title prop of the callout. I don't think we need the Name / type change

I don't mean to block this either. So I would prefer seeing either both a callout + modal confirmation or just the modal confirmation.

I spoke to the runtime fields ui working group about this and we've decided to go with the single line warning callout with icon.

updated description with new screenshot

mattkime · 2021-04-08T12:48:20Z

@elasticmachine merge upstream

Dosant

Code LGTM,
I agree with https://github.com/elastic/kibana/pull/95528/files#r608551323 that error label doesn't seem like the best fit here

mattkime · 2021-04-09T15:14:45Z

@elasticmachine merge upstream

mattkime · 2021-04-12T14:30:16Z

@elasticmachine merge upstream

mdefazio

LGTM

gchaps · 2021-04-13T20:47:57Z

Copy LGTM

kibanamachine · 2021-04-13T20:50:45Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`indexPatternFieldEditor`	21.2KB	22.1KB	+974.0B

History

💚 Build #118745 succeeded 7000d7f
💚 Build #118530 succeeded 968eba0
💛 Build #118212 was flaky 9510215
💛 Build #118110 was flaky 1c5beac
💔 Build #118098 failed ecf643d

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…ax_primary_shard_size * 'master' of github.com:elastic/kibana: (99 commits) added missing optional chain for bracket notation (elastic#96939) [Discover][DocViewer] Fix toggle columns from doc viewer table tab (elastic#95748) [TSVB] Fix per-request caching of index patterns (elastic#97043) [Datatable] Fix filter cell flakiness (elastic#96934) Unskip heatmap suite and fixes flakiness (elastic#96941) [Fleet] Improve performance of data stream API (elastic#97058) [ML] Data Frame Analytics: remove beta badge (elastic#96977) [App Search] Migrate expanded rows for meta engines table in Engines Overview (elastic#96251) Instances latency distribution chart tooltips and axis fixes (elastic#95577) [Monitoring] Using primary average shard size (elastic#96177) [Workplace Search] Hide Kibana chrome on 3rd party connector redirects (elastic#97028) ## [Security Solution] Fixes `Exit full screen` and `Copy to cliboard` styling issues (elastic#96676) Index pattern field editor - Add warning on name or type change (elastic#95528) [App Search] Add small engine breadcrumb utility helper (elastic#96917) Copy esArchiver commands from ./reassign.ts to fix tests (elastic#97012) [Security Solution][Detections] Updates MITRE Tactics, Techniques, and Subtechniques for 7.13 (elastic#97011) Index patterns server - throw correct error on field caps 404 (elastic#95879) Use `EuiThemeProvider` in lists plugin tests and stories (elastic#96129) [npm] upgrade caniuse database (elastic#97002) chore(NA): moving @kbn/apm-utils into bazel (elastic#96227) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/serialization/policy_serialization.test.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/schema.ts

…to-metrics-tab * 'master' of github.com:elastic/kibana: (61 commits) [Usage collection] Usage counters (elastic#96696) UI actions readme (elastic#96925) [TSVB] Enable brush for visualizations created with no index patterns (elastic#96727) [Data telemetry] Add Async Search to the tests (elastic#96693) added missing optional chain for bracket notation (elastic#96939) [Discover][DocViewer] Fix toggle columns from doc viewer table tab (elastic#95748) [TSVB] Fix per-request caching of index patterns (elastic#97043) [Datatable] Fix filter cell flakiness (elastic#96934) Unskip heatmap suite and fixes flakiness (elastic#96941) [Fleet] Improve performance of data stream API (elastic#97058) [ML] Data Frame Analytics: remove beta badge (elastic#96977) [App Search] Migrate expanded rows for meta engines table in Engines Overview (elastic#96251) Instances latency distribution chart tooltips and axis fixes (elastic#95577) [Monitoring] Using primary average shard size (elastic#96177) [Workplace Search] Hide Kibana chrome on 3rd party connector redirects (elastic#97028) ## [Security Solution] Fixes `Exit full screen` and `Copy to cliboard` styling issues (elastic#96676) Index pattern field editor - Add warning on name or type change (elastic#95528) [App Search] Add small engine breadcrumb utility helper (elastic#96917) Copy esArchiver commands from ./reassign.ts to fix tests (elastic#97012) [Security Solution][Detections] Updates MITRE Tactics, Techniques, and Subtechniques for 7.13 (elastic#97011) ...

kibanamachine · 2021-04-15T21:00:18Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 95528 or prevent reminders by adding the backport:skip label.

kibanamachine · 2021-04-16T21:10:17Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 95528 or prevent reminders by adding the backport:skip label.

…tic#95528) * add warning on name or type change

…) (#97386) * add warning on name or type change Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

add warning on name or type change

1ff49bf

mattkime changed the title ~~add warning on name or type change~~ Index pattern field editor - Add warning on name or type change Mar 26, 2021

mattkime added v7.13.0 v8.0.0 labels Mar 26, 2021

add functional test

a4542a6

Merge branch 'master' into runtime_field_name_type_change_warning

b90841c

mattkime added Team:AppServices Feature:Data Views Data Views code and UI - index patterns before 8.0 Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages release_note:skip Skip the PR/issue when compiling release notes labels Apr 5, 2021

Merge branch 'master' into runtime_field_name_type_change_warning

28b15e0

sebelga reviewed Apr 5, 2021

View reviewed changes

src/plugins/index_pattern_field_editor/public/components/field_editor/field_editor.tsx Outdated Show resolved Hide resolved

mattkime added 2 commits April 5, 2021 10:06

Merge branch 'master' into runtime_field_name_type_change_warning

85e8048

fix name / type field change events

a361912

mattkime requested a review from sebelga April 5, 2021 19:40

mattkime marked this pull request as ready for review April 5, 2021 19:40

mattkime requested a review from a team as a code owner April 5, 2021 19:40

Update field_editor.tsx

4c2a70d

Merge branch 'master' into runtime_field_name_type_change_warning

190f996

mattkime requested a review from Dosant April 6, 2021 21:12

gchaps reviewed Apr 6, 2021

View reviewed changes

src/plugins/index_pattern_field_editor/public/components/field_editor/field_editor.tsx Outdated Show resolved Hide resolved

Update src/plugins/index_pattern_field_editor/public/components/field…

e6fc43b

…_editor/field_editor.tsx Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>

sebelga approved these changes Apr 7, 2021

View reviewed changes

Merge branch 'master' into runtime_field_name_type_change_warning

39ca842

mattkime added 2 commits April 7, 2021 21:28

Update field_editor.tsx

ecf643d

fix unit test

1c5beac

Merge branch 'master' into runtime_field_name_type_change_warning

9510215

Dosant approved these changes Apr 8, 2021

View reviewed changes

Merge branch 'master' into runtime_field_name_type_change_warning

968eba0

kibanamachine and others added 3 commits April 12, 2021 10:30

Merge branch 'master' into runtime_field_name_type_change_warning

7000d7f

Merge branch 'master' into runtime_field_name_type_change_warning

fcc3d00

use warning instead of normal text

a6d312c

mdefazio approved these changes Apr 13, 2021

View reviewed changes

mattkime merged commit 1cd90d7 into elastic:master Apr 13, 2021

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 15, 2021

mattkime mentioned this pull request Apr 17, 2021

[7.x] Index pattern field editor - Add warning on name or type change (#95528) #97386

Merged

mattkime added a commit to mattkime/kibana that referenced this pull request Apr 17, 2021

Index pattern field editor - Add warning on name or type change (elas…

0754b33

…tic#95528) * add warning on name or type change

mattkime added a commit that referenced this pull request Apr 17, 2021

Index pattern field editor - Add warning on name or type change (#95528…

884e931

…) (#97386) * add warning on name or type change Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 17, 2021

Conversation

mattkime commented Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

mattkime commented Mar 26, 2021

Uh oh!

mattkime commented Mar 27, 2021

Uh oh!

mattkime commented Apr 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticmachine commented Apr 5, 2021

Uh oh!

sebelga commented Apr 6, 2021

Uh oh!

Uh oh!

sebelga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattkime Apr 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdefazio Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattkime commented Apr 8, 2021

Uh oh!

Dosant left a comment

Choose a reason for hiding this comment

Uh oh!

mattkime commented Apr 9, 2021

Uh oh!

mattkime commented Apr 12, 2021

Uh oh!

mdefazio left a comment

Choose a reason for hiding this comment

Uh oh!

gchaps commented Apr 13, 2021

Uh oh!

kibanamachine commented Apr 13, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

Uh oh!

kibanamachine commented Apr 15, 2021

Uh oh!

kibanamachine commented Apr 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

mattkime commented Mar 26, 2021 •

edited

Loading

mattkime Apr 11, 2021 •

edited

Loading

mdefazio Apr 12, 2021 •

edited

Loading