[ILM] Change "wait for snapshot" policy text field to EuiCombobox by yuliacech · Pull Request #70627 · elastic/kibana

yuliacech · 2020-07-02T16:50:07Z

Summary

This PR updates "wait for snapshot" text field in Delete phase of an index lifecycle policy to use a EuiCombobox.

Selecting a policy from the list

Input value doesn't match any existing policies

No snapshot policies exist

In the next PR, I will add a link to the words "snapshot policies" that navigates to SLM.

Error loading policies (with a reload button)

How to test

Start es locally with a repo path argument: yarn es snapshot -E path.repo=./my_repo_test
Create a repository and several snapshot policies in "Snapshot and Restore" UI
Create or edit an existing ILM policy and activate "Delete phase"

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser

Release Note

We updated the snapshot policy name field in Delete phase of index lifecycle policy. This component now display a list of existing snapshot policies and warns the user if their input doesn't match any existing policies.

elasticmachine · 2020-07-02T17:04:04Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

yuliacech · 2020-07-03T09:06:02Z

@elasticmachine merge upstream

…elete phase

…st tests

jloleysens · 2020-07-07T08:23:34Z

@elasticmachine merge upstream

jloleysens

This is looking really good @yuliacech and thanks for the cleanup you did too!

Before approving (everything looks to be mostly in order!) I left a few suggestions and nits (nothing to block on) but just wanted remark on the UX:

Currently, when clicking in the combobox to start typing the list of available snapshots does not appear. Only when I start typing do the suggestions appear. As a user it would be nice to have all the information before I start typing so that I don't have to try and remember what snapshots policies I currently have. I think this might be caused by use of noSuggestions.
I really like the addition of the callouts you added, but I think it competes with what the combobox already gives us. If I type in a snapshot policy name that does not match an existing policy I get the screenshot below. This kind of already indicates that I am going off track in terms of policy names and I think the copy in the description already guides the user to the exact use of this field. My inclination is to say that we can remove the callouts and keep just the one that says we could not retrieve any policy names for the autocomplete. Although I think if we are in that position ILM policy page would also not have loaded in the first place 😅

Non-matching policy name output

Let me know what you think!

...s/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.helpers.tsx

...ugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.test.ts

...cle_management/public/application/sections/edit_policy/components/snapshot_policies/index.ts

x-pack/plugins/index_lifecycle_management/public/application/services/api.ts

...ugins/index_lifecycle_management/server/routes/api/snapshot_policies/register_fetch_route.ts

...t/public/application/sections/edit_policy/components/snapshot_policies/snapshot_policies.tsx

yuliacech · 2020-07-07T11:59:38Z

@jloleysens Hi Jean-Louis, thank you so much for your detailed review! I agree with your code improvement suggestions and will get esdocs team to have a look at the copy.

I'm trying to figure out why the option list is not shown before the user types anything in. Doesn't seem to be caused by noSuggestions, maybe something with the focus is wrong 🤔
I agree that customPolicy callout might be an overkill but I'm not sure that the hard-coded combo box text "Hit Enter ..." makes sense here either. According to the EUI team, it can't be changed currently.

jloleysens · 2020-07-07T14:49:51Z

I'm trying to figure out why the option list is not shown before the user types anything in. Doesn't seem to be caused by noSuggestions, maybe something with the focus is wrong 🤔

It might be some edge case bug that we are running into, but they seem to be working as expected here: https://elastic.github.io/eui/#/forms/combo-box. I'm sure the EUI team could help out!

yuliacech · 2020-07-07T15:20:31Z

@jloleysens I ended up opening an issue in eui repo, because it does seem like some weird bug/edge case.
I can solve the issue when there is no value, so the options list drops down on click. But when there is any value in the combobox the options will only show after deleting the input or starting typing.

jloleysens

Overall looks good, happy that we could improve the UX of the combobox slightly (did not re-test this though).

It would be good to bring the code a bit more in line with our other patterns per the comments and get copy review from esdocs. Code-wise LGTM.

lockewritesdocs

Added some suggested revisions to the UI text for these changes.

...t/public/application/sections/edit_policy/components/snapshot_policies/snapshot_policies.tsx

yuliacech · 2020-07-08T11:53:59Z

@elasticmachine merge upstream

…ections/edit_policy/components/snapshot_policies/snapshot_policies.tsx Co-authored-by: Adam Locke <adam.locke@elastic.co>

yuliacech · 2020-07-08T16:41:13Z

@elasticmachine merge upstream

yuliacech · 2020-07-09T10:20:04Z

@elasticmachine merge upstream

… wait_for_snapshot_dropdown

yuliacech · 2020-07-09T11:58:33Z

@elasticmachine merge upstream

yuliacech · 2020-07-09T13:40:40Z

@elasticmachine merge upstream

yuliacech · 2020-07-09T16:14:41Z

@elasticmachine merge upstream

kibanamachine · 2020-07-09T18:12:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4023ee1

Build metrics

✅ unchanged

History

💔 Build #60213 failed 1daa1af
💔 Build #60164 failed 8d4e701
💔 Build #60144 failed 0213793
💚 Build #60130 succeeded 80091c8
💔 Build #59836 failed d72a5c3

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

…astic#70627) * [ILM] Change "Wait for snapshot policy" text field to a dropdown in Delete phase * [ILM] Change "wait for snapshot" field to a EuiCombobox and update jest tests * [ILM] Update jest tests to check callouts * [ILM] Implement PR review suggestions * Update x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/snapshot_policies/snapshot_policies.tsx Co-authored-by: Adam Locke <adam.locke@elastic.co> * Update x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/snapshot_policies/snapshot_policies.tsx Co-authored-by: Adam Locke <adam.locke@elastic.co> * Update x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/snapshot_policies/snapshot_policies.tsx Co-authored-by: Adam Locke <adam.locke@elastic.co> * [ILM] Fix copy * [ILM] Fix copy * [ILM] Fix build error * [ILM] Delete periods in callout titles Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Adam Locke <adam.locke@elastic.co>

…0627) (#71293) * [ILM] Change "Wait for snapshot policy" text field to a dropdown in Delete phase * [ILM] Change "wait for snapshot" field to a EuiCombobox and update jest tests * [ILM] Update jest tests to check callouts * [ILM] Implement PR review suggestions * Update x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/snapshot_policies/snapshot_policies.tsx Co-authored-by: Adam Locke <adam.locke@elastic.co> * Update x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/snapshot_policies/snapshot_policies.tsx Co-authored-by: Adam Locke <adam.locke@elastic.co> * Update x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/snapshot_policies/snapshot_policies.tsx Co-authored-by: Adam Locke <adam.locke@elastic.co> * [ILM] Fix copy * [ILM] Fix copy * [ILM] Fix build error * [ILM] Delete periods in callout titles Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Adam Locke <adam.locke@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Adam Locke <adam.locke@elastic.co>

yuliacech marked this pull request as ready for review July 2, 2020 16:53

yuliacech requested a review from a team as a code owner July 2, 2020 16:53

yuliacech added Feature:ILM release_note:enhancement Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// v7.9.0 v8.0.0 labels Jul 2, 2020

yuliacech force-pushed the wait_for_snapshot_dropdown branch from 86eecd9 to 4c6b46a Compare July 3, 2020 09:33

yuliacech added 3 commits July 6, 2020 13:06

[ILM] Change "Wait for snapshot policy" text field to a dropdown in D…

f022f4b

…elete phase

[ILM] Change "wait for snapshot" field to a EuiCombobox and update je…

be93e2c

…st tests

[ILM] Update jest tests to check callouts

dd5409a

yuliacech force-pushed the wait_for_snapshot_dropdown branch from 4c6b46a to dd5409a Compare July 6, 2020 11:07

jloleysens self-requested a review July 6, 2020 16:01

Merge branch 'master' into wait_for_snapshot_dropdown

1887af4

jloleysens reviewed Jul 7, 2020

View reviewed changes

...t/public/application/sections/edit_policy/components/snapshot_policies/snapshot_policies.tsx Outdated Show resolved Hide resolved

jloleysens approved these changes Jul 7, 2020

View reviewed changes

lockewritesdocs self-requested a review July 7, 2020 15:34

[ILM] Implement PR review suggestions

51e8bdd

yuliacech mentioned this pull request Jul 7, 2020

[EuiComboBox] Add an option to configure wording and display of "Hit Enter to add ... as a custom option" elastic/eui#3702

Closed

lockewritesdocs approved these changes Jul 7, 2020

View reviewed changes

elasticmachine and others added 2 commits July 8, 2020 05:54

Merge branch 'master' into wait_for_snapshot_dropdown

97ee3ed

Update x-pack/plugins/index_lifecycle_management/public/application/s…

1c91ac5

…ections/edit_policy/components/snapshot_policies/snapshot_policies.tsx Co-authored-by: Adam Locke <adam.locke@elastic.co>

yuliacech and others added 4 commits July 8, 2020 15:02

Update x-pack/plugins/index_lifecycle_management/public/application/s…

58c52e8

…ections/edit_policy/components/snapshot_policies/snapshot_policies.tsx Co-authored-by: Adam Locke <adam.locke@elastic.co>

Update x-pack/plugins/index_lifecycle_management/public/application/s…

ada74dd

…ections/edit_policy/components/snapshot_policies/snapshot_policies.tsx Co-authored-by: Adam Locke <adam.locke@elastic.co>

[ILM] Fix copy

890f19c

[ILM] Fix copy

c18d0d1

elasticmachine and others added 2 commits July 8, 2020 10:41

Merge branch 'master' into wait_for_snapshot_dropdown

d72a5c3

[ILM] Fix build error

765cbe8

elasticmachine and others added 3 commits July 9, 2020 04:20

Merge branch 'master' into wait_for_snapshot_dropdown

80091c8

[ILM] Delete periods in callout titles

bf864ad

Merge remote-tracking branch 'origin/wait_for_snapshot_dropdown' into…

0213793

… wait_for_snapshot_dropdown

Merge branch 'master' into wait_for_snapshot_dropdown

8d4e701

Merge branch 'master' into wait_for_snapshot_dropdown

1daa1af

Merge branch 'master' into wait_for_snapshot_dropdown

4023ee1

yuliacech merged commit 733f338 into elastic:master Jul 9, 2020

yuliacech deleted the wait_for_snapshot_dropdown branch July 9, 2020 18:37

yuliacech restored the wait_for_snapshot_dropdown branch July 9, 2020 18:37

yuliacech mentioned this pull request Jul 9, 2020

[7.x] [ILM] Change "wait for snapshot" policy text field to EuiCombobox (#70627) #71293

Merged

yuliacech deleted the wait_for_snapshot_dropdown branch July 20, 2020 13:30

Conversation

yuliacech commented Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Selecting a policy from the list

Input value doesn't match any existing policies

No snapshot policies exist

Error loading policies (with a reload button)

How to test

Checklist

Release Note

Uh oh!

elasticmachine commented Jul 2, 2020

Uh oh!

yuliacech commented Jul 3, 2020

Uh oh!

jloleysens commented Jul 7, 2020

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yuliacech commented Jul 7, 2020

Uh oh!

jloleysens commented Jul 7, 2020

Uh oh!

yuliacech commented Jul 7, 2020

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

lockewritesdocs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yuliacech commented Jul 8, 2020

Uh oh!

yuliacech commented Jul 8, 2020

Uh oh!

yuliacech commented Jul 9, 2020

Uh oh!

yuliacech commented Jul 9, 2020

Uh oh!

yuliacech commented Jul 9, 2020

Uh oh!

yuliacech commented Jul 9, 2020

Uh oh!

kibanamachine commented Jul 9, 2020

💚 Build Succeeded

Build metrics

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yuliacech commented Jul 2, 2020 •

edited

Loading