[ILM] Convert node allocation component to TS and use hooks #72888

yuliacech · 2020-07-22T15:39:20Z

Summary

This PR is a part of an ongoing effort to convert ILM codebase from JavaScript to TypeScript. In this PR, NodeAllocation component of the ILM policy edit page (in warm and cold phases) has been refactored. As a result, the component will not use the redux store for loaded node attributes anymore, but it will use a hook for that. Also a callout was added for the case, when the request fails.

Error loading node attributes

Before

Loading indicator never disappears.
No way to try reloading node attributes.

After

Loading indicator is only shown while the request is in progress.
A callout was added where the user can try to reload node attributes.

Checklist

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

yuliacech · 2020-07-23T14:35:12Z

@elasticmachine merge upstream

elasticmachine · 2020-07-24T15:50:29Z

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

yuliacech · 2020-07-28T09:33:10Z

@elasticmachine merge upstream

alisonelizabeth

Nice work @yuliacech! This is a great improvement. I left a few comments in the code. I think some of the things I noticed were pre-existing, but would be nice to address as part of the refactor.

...lugins/index_lifecycle_management/public/application/sections/components/learn_more_link.tsx

...ement/public/application/sections/edit_policy/components/node_allocation/node_allocation.tsx

x-pack/plugins/index_lifecycle_management/__jest__/components/edit_policy.test.js

...ement/public/application/sections/edit_policy/components/node_allocation/node_allocation.tsx

yuliacech · 2020-07-29T14:11:18Z

@elasticmachine merge upstream

yuliacech · 2020-07-29T16:30:16Z

Hi @alisonelizabeth ,
thank you for such a detailed code review!
I implemented everything you commented on, could you please have another look?

alisonelizabeth

Thanks @yuliacech for making the suggested changes! Overall LGTM.

I found a couple other minor things when I did a second round of testing. I think it would be nice if they could be addressed before merging, but I'm going to go ahead and approve to not block you.

Invalid doc link (commented in code)
I didn't check if this is a regression or not, but the "View a list of nodes..." link doesn't appear to be aligned correctly when I have node attributes configured.

[nit] The error callout is a little hard for me to parse with the status code, error string, and error message in the same <p> tag. It may be worth reaching out to design - or maybe the docs team is more appropriate - for feedback.

...ement/public/application/sections/edit_policy/components/node_allocation/node_allocation.tsx

… component

yuliacech · 2020-07-30T13:54:23Z

Great suggestions, @alisonelizabeth !
I fixed the docs link and the "View list ..." link.

There was also a type check error, so I added a type for the api error:

kibana/x-pack/plugins/index_lifecycle_management/public/application/services/http.ts

Line 45 in 1ccf12f

    
           export const useRequest = <T = any, E = { statusCode: number; error: string; message: string }>(

yuliacech · 2020-07-30T14:08:31Z

Hello @esdocs team,

I added a new callout instead of a toast notification when there is an error loading node attributes. I used the same wording for error information but it seems a bit hard to read. So I was hoping for some feedback to improve it.

Toast notification before:

New callout:

The text is build like this: {statusCode}: {errorString}. {message} where statusCode is 404, 500 etc, errorString is "Bad Request", "Internal Server Error" etc and message is the information from API error but can be the same as errorString.
Some examples of current callout wordings:

yuliacech · 2020-07-30T14:54:36Z

@elasticmachine merge upstream

yuliacech · 2020-08-03T12:13:31Z

@elasticmachine merge upstream

lockewritesdocs · 2020-08-04T13:08:16Z

@yuliacech, thanks for the ping 👍 I’m curious whether we want to display {errorString} if we’re essentially restating the same information in the {message}. I suggest refocusing the content to put the message first, removing {errorString}, and putting the {statusCode} last.

Pinging @probakowski and @leehinman to provide any additional context that might be useful to the user in understanding these messages.

Unable to load node attributes
Elasticsearch failed to load node attributes (400).

Unable to load node attributes
Elasticsearch cannot find node attributes (404).

Unable to load node attributes
Elasticsearch encountered an internal server error (500).

yuliacech · 2020-08-05T12:28:11Z

@elasticmachine merge upstream

yuliacech · 2020-08-06T13:17:38Z

@elasticmachine merge upstream

yuliacech · 2020-08-06T13:43:50Z

Thanks a lot for your suggestion, @lockewritesdocs !
I updated how error is displayed, would you mind having another look?

lockewritesdocs

Left a very minor suggestion, but 👍 otherwise!

lockewritesdocs · 2020-08-06T14:08:38Z

...ement/public/application/sections/edit_policy/components/node_allocation/node_allocation.tsx

+          title={
+            <FormattedMessage
+              id="xpack.indexLifecycleMgmt.editPolicy.nodeAttributesLoadingFailedTitle"
+              defaultMessage="Unable to load node attributes."


Suggest removing the period here, but that's totally minor. If having a period at the end matches other Kibana error message conventions, then please ignore.

kibanamachine · 2020-08-06T16:04:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3841a11

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
indexLifecycleManagement	177	-1	178

async chunks size

id	value	diff	baseline
indexLifecycleManagement	268.5KB	-6.5KB	275.0KB

page load bundle size

id	value	diff	baseline
indexLifecycleManagement	238.3KB	-375.0B	238.7KB

History

💚 Build #66893 succeeded 594d017
💚 Build #66618 succeeded 1f04f67
💚 Build #66059 succeeded 062465a
💚 Build #65570 succeeded 739be48
💔 Build #65545 failed a557a5b

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

…72888) * [ILM] Convert node allocation component to TS and use hooks * [ILM] Fix jest tests * [ILM] Fix i18n check * [ILM] Implement code review suggestions * [ILM] Fix type check, docs link and button maxWidth in NodeAllocation component * Fix internaliation error * [ILM] Change error message when unable to load node attributes * [ILM] Delete a period in error callout Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…74556) * [ILM] Convert node allocation component to TS and use hooks * [ILM] Fix jest tests * [ILM] Fix i18n check * [ILM] Implement code review suggestions * [ILM] Fix type check, docs link and button maxWidth in NodeAllocation component * Fix internaliation error * [ILM] Change error message when unable to load node attributes * [ILM] Delete a period in error callout Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

yuliacech added Feature:ILM v7.10.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more labels Jul 22, 2020

yuliacech added 3 commits July 23, 2020 18:56

[ILM] Convert node allocation component to TS and use hooks

1781ca7

[ILM] Fix jest tests

9958274

[ILM] Fix i18n check

4486489

yuliacech force-pushed the ilm_typescript branch from b3d4979 to 4486489 Compare July 23, 2020 16:57

yuliacech marked this pull request as ready for review July 24, 2020 15:50

yuliacech requested a review from a team as a code owner July 24, 2020 15:50

Merge branch 'master' into ilm_typescript

af77efa

alisonelizabeth reviewed Jul 28, 2020

View reviewed changes

elasticmachine and others added 3 commits July 29, 2020 08:11

Merge branch 'master' into ilm_typescript

653ff5e

Merge branch 'master' into ilm_typescript

4cc9584

[ILM] Implement code review suggestions

fc0633e

alisonelizabeth approved these changes Jul 30, 2020

View reviewed changes

...ement/public/application/sections/edit_policy/components/node_allocation/node_allocation.tsx Outdated Show resolved Hide resolved

[ILM] Fix type check, docs link and button maxWidth in NodeAllocation…

1ccf12f

… component

yuliacech requested a review from esdocs July 30, 2020 13:54

elasticmachine and others added 2 commits July 30, 2020 08:54

Merge branch 'master' into ilm_typescript

a557a5b

Fix internaliation error

739be48

Merge branch 'master' into ilm_typescript

062465a

Merge branch 'master' into ilm_typescript

1f04f67

elasticmachine and others added 2 commits August 6, 2020 07:17

Merge branch 'master' into ilm_typescript

594d017

[ILM] Change error message when unable to load node attributes

22c6b86

lockewritesdocs approved these changes Aug 6, 2020

View reviewed changes

[ILM] Delete a period in error callout

3841a11

yuliacech merged commit d764499 into elastic:master Aug 6, 2020

yuliacech deleted the ilm_typescript branch August 6, 2020 16:06

yuliacech restored the ilm_typescript branch August 6, 2020 16:06

yuliacech mentioned this pull request Aug 6, 2020

[7.x] [ILM] Convert node allocation component to TS and use hooks (#72888) #74556

Merged

yuliacech deleted the ilm_typescript branch August 6, 2020 17:53

yuliacech mentioned this pull request Sep 18, 2020

Index Lifecycle Policies "Nodes info" button is misaligned #56703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ILM] Convert node allocation component to TS and use hooks #72888

[ILM] Convert node allocation component to TS and use hooks #72888

yuliacech commented Jul 22, 2020 •

edited

Loading

yuliacech commented Jul 23, 2020

elasticmachine commented Jul 24, 2020

yuliacech commented Jul 28, 2020

alisonelizabeth left a comment

yuliacech commented Jul 29, 2020

yuliacech commented Jul 29, 2020

alisonelizabeth left a comment

yuliacech commented Jul 30, 2020 •

edited

Loading

yuliacech commented Jul 30, 2020

yuliacech commented Jul 30, 2020

yuliacech commented Aug 3, 2020

lockewritesdocs commented Aug 4, 2020

yuliacech commented Aug 5, 2020

yuliacech commented Aug 6, 2020

yuliacech commented Aug 6, 2020

lockewritesdocs left a comment

lockewritesdocs Aug 6, 2020

kibanamachine commented Aug 6, 2020

[ILM] Convert node allocation component to TS and use hooks #72888

[ILM] Convert node allocation component to TS and use hooks #72888

Conversation

yuliacech commented Jul 22, 2020 • edited Loading

Summary

Error loading node attributes

Before

After

Checklist

yuliacech commented Jul 23, 2020

elasticmachine commented Jul 24, 2020

yuliacech commented Jul 28, 2020

alisonelizabeth left a comment

Choose a reason for hiding this comment

yuliacech commented Jul 29, 2020

yuliacech commented Jul 29, 2020

alisonelizabeth left a comment

Choose a reason for hiding this comment

yuliacech commented Jul 30, 2020 • edited Loading

yuliacech commented Jul 30, 2020

yuliacech commented Jul 30, 2020

yuliacech commented Aug 3, 2020

lockewritesdocs commented Aug 4, 2020

yuliacech commented Aug 5, 2020

yuliacech commented Aug 6, 2020

yuliacech commented Aug 6, 2020

lockewritesdocs left a comment

Choose a reason for hiding this comment

lockewritesdocs Aug 6, 2020

Choose a reason for hiding this comment

kibanamachine commented Aug 6, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

yuliacech commented Jul 22, 2020 •

edited

Loading

yuliacech commented Jul 30, 2020 •

edited

Loading