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

[ILM] Added a flyout with linked index templates #106734

Merged
merged 18 commits into from
Jul 30, 2021

Conversation

yuliacech
Copy link
Contributor

@yuliacech yuliacech commented Jul 26, 2021

Summary

This PR adds a new column "Linked index templates" to the ILM policies list. A click on the number in this columns opens a flyout with a list of linked index templates linked to the ILM policy. The index template names are links to the Index Management view with index templates details.
Future PRs for this feature will include an update of the index templates column after adding an ILM policy to an index template, a search bar for index templates list, number of index templates in the delete confirmation and on the edit page.

Changes to the codebase

  1. useHistory hook instead of passing history as prop
  2. added getUrlForApp and navigateToApp functions to useKibana context instead of passing them as prop
  3. use RedirectAppLinks so that there is no page reload when navigating to other plugins

Screenshots

ILM policies list with a new "Linked index templates" column

Screenshot 2021-07-30 at 10 55 40

Index templates flyout

Screenshot 2021-07-30 at 10 56 09

Flyout video

index_templates_screencast.mov

Checklist

Delete any items that are not applicable to this PR.

Release Note

There is now a flyout in the Index Lifecycle Management app that displays a list of index templates linked to the Index Lifecycle policy.

Screenshot 2021-07-30 at 10 55 40

@yuliacech yuliacech changed the title Ilm linked templates [ILM] Added a flyout with linked index templates Jul 26, 2021
@yuliacech yuliacech added enhancement New value added to drive a business result release_note:enhancement Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.15.0 v8.0.0 Feature:ILM labels Jul 26, 2021
@yuliacech
Copy link
Contributor Author

@elasticmachine merge upstream

@yuliacech
Copy link
Contributor Author

@elasticmachine merge upstream

@yuliacech yuliacech marked this pull request as ready for review July 27, 2021 15:01
@yuliacech yuliacech requested a review from a team as a code owner July 27, 2021 15:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-management (Team:Stack Management)

@yuliacech yuliacech requested review from sabarasaba and a team July 27, 2021 15:01
Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

Nice work @yuliacech! I tested locally and it all works alright, and code changes LGTM. I have though a few UX suggestions:

Enhance flyout table

Since we have a lot of space available in the table, I think would be a nice addition to add the template type (system, cloud, managed). When I was testing it locally I firstly opened an index template that was from system and since its not shown by default I was a bit confused about it.

Screenshot 2021-07-28 at 11 23 12

Empty state

Unsure about this one, but maybe would look nicer if we were to use something like an EmptyPrompt instead of an empty table
Screenshot 2021-07-28 at 11 19 03

Missing padding for close flyout cta

I think this might be an eui bug but the close flyout cta seems to not have enough padding on the sides
Screenshot 2021-07-28 at 12 15 49

@@ -181,8 +194,20 @@ describe('policy table', () => {
test('displays policy properties', () => {
const rendered = mountWithIntl(component);
const firstRow = findTestSubject(rendered, 'policyTableRow').at(0).text();
Copy link
Member

Choose a reason for hiding this comment

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

I think this data-test-subj now changed to policyTableRow-${name} so might need a little refactor in this part too

Copy link
Contributor

@debadair debadair left a comment

Choose a reason for hiding this comment

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

Left one suggestion, but otherwise LGTM. I would definitely remove policy from the flyout header. I think it would be helpful to use the header to provide info about what "linked to" means, rather than just repeating the linked to phrase.

@debadair
Copy link
Contributor

Empty state

Unsure about this one, but maybe would look nicer if we were to use something like an EmptyPrompt instead of an empty table

Seems like an entry of 0 in the linked templates shouldn't even open the flyout?

@yuliacech
Copy link
Contributor Author

@elasticmachine merge upstream

@yuliacech
Copy link
Contributor Author

Thanks a lot for your review, @sabarasaba!
I disabled the flyout when there are no index templates and I'll consult with Dmitry for a better UI for an empty state and about adding more info to the flyout. The close button padding seems to be reduced on purpose to align it to the padding of the flyout itself. Could you please have another look?

@yuliacech
Copy link
Contributor Author

Thanks a lot for the review, @debadair!
I applied your suggestions for flyout title wording and disabled it for when there are no linked index templates.

Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

Latest changes lgtm @yuliacech, nice job!

@yuliacech yuliacech merged commit c6420fb into elastic:master Jul 30, 2021
yuliacech added a commit to yuliacech/kibana that referenced this pull request Jul 30, 2021
* [ILM] Server to use new in_use_by property returned by ES API

* [ILM] Cleaning up the PR changes

* [ILM] Fixed functional test

* [ILM] Fixed 'modifiedDate' display in the table

* [ILM] Fixed sorting test

* [ILM] Removed a not needed function declaration

* [ILM] Added index templates flyout to the policies list

* [ILM] Added test for the index templates flyout

* [ILM] Added an a11y test for the index templates flyout

* Update x-pack/plugins/index_lifecycle_management/public/application/components/index_templates_flyout.tsx

Co-authored-by: debadair <debadair@elastic.co>

* [ILM] Fixed jest test and made 0 index templates to not open the flyout

* [ILM] Fixed a11y test

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: debadair <debadair@elastic.co>
yuliacech added a commit that referenced this pull request Jul 30, 2021
* [ILM] Server to use new in_use_by property returned by ES API

* [ILM] Cleaning up the PR changes

* [ILM] Fixed functional test

* [ILM] Fixed 'modifiedDate' display in the table

* [ILM] Fixed sorting test

* [ILM] Removed a not needed function declaration

* [ILM] Added index templates flyout to the policies list

* [ILM] Added test for the index templates flyout

* [ILM] Added an a11y test for the index templates flyout

* Update x-pack/plugins/index_lifecycle_management/public/application/components/index_templates_flyout.tsx

Co-authored-by: debadair <debadair@elastic.co>

* [ILM] Fixed jest test and made 0 index templates to not open the flyout

* [ILM] Fixed a11y test

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: debadair <debadair@elastic.co>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: debadair <debadair@elastic.co>
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
indexLifecycleManagement 211 212 +1

Async chunks

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

id before after diff
indexLifecycleManagement 246.3KB 248.2KB +1.8KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
indexLifecycleManagement 49.8KB 48.6KB -1.2KB
indexManagement 36.9KB 37.0KB +99.0B
total -1.1KB
Unknown metric groups

API count

id before after diff
indexManagement 162 165 +3

API count missing comments

id before after diff
indexManagement 157 160 +3

History

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

streamich pushed a commit to vadimkibana/kibana that referenced this pull request Aug 8, 2021
* [ILM] Server to use new in_use_by property returned by ES API

* [ILM] Cleaning up the PR changes

* [ILM] Fixed functional test

* [ILM] Fixed 'modifiedDate' display in the table

* [ILM] Fixed sorting test

* [ILM] Removed a not needed function declaration

* [ILM] Added index templates flyout to the policies list

* [ILM] Added test for the index templates flyout

* [ILM] Added an a11y test for the index templates flyout

* Update x-pack/plugins/index_lifecycle_management/public/application/components/index_templates_flyout.tsx

Co-authored-by: debadair <debadair@elastic.co>

* [ILM] Fixed jest test and made 0 index templates to not open the flyout

* [ILM] Fixed a11y test

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: debadair <debadair@elastic.co>
@yuliacech yuliacech deleted the ilm_linked_templates branch November 9, 2021 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:ILM release_note:enhancement Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants