-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] TS conversion of policies table #76006
Conversation
@elasticmachine merge upstream |
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yuliacech this is looking really great! Happy to see us able to remove dependence on redux store! I did a local smoke test and the plugin seems to be working as expected!
Before approving I just wanted to get your input on the selected policies state that seems to not be in use in policy_table.tsx
.
Otherwise looks GTG!
initUiMetric({ reportUiStats: () => {} }); | ||
// This expects HttpSetup but we're giving it AxiosInstance. | ||
// @ts-ignore | ||
initHttp(axios.create({ adapter: axiosXhrAdapter })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not pass in HttpSetup
here? We should be able to import and create a mock from src/core/public/mocks
- similar to what we have for usageCollectionPluginMock
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason I couldn't get the test to work with coreMock
but then I saw HttpService
being used in some other tests, so I tried it and it worked.
...x_lifecycle_management/public/application/sections/policy_table/components/table_content.tsx
Outdated
Show resolved
Hide resolved
...x_lifecycle_management/public/application/sections/policy_table/components/table_content.tsx
Outdated
Show resolved
Hide resolved
...x_lifecycle_management/public/application/sections/policy_table/components/table_content.tsx
Outdated
Show resolved
Hide resolved
...dex_lifecycle_management/public/application/sections/policy_table/policy_table.container.tsx
Outdated
Show resolved
Hide resolved
...plugins/index_lifecycle_management/public/application/sections/policy_table/policy_table.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/index_lifecycle_management/public/application/services/filter_items.ts
Outdated
Show resolved
Hide resolved
Hi @jloleysens , thanks a lot for your review! I deleted unused |
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
page load bundle size
distributable file count
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @yuliacech ! Thanks for addressing my feedback.
Did not retest locally, but the latest commits look good to me!
* [ILM] TS conversion of policies table * [ILM] Fix type check errors * [ILM] Fix i18n check errors * [ILM] Add types to linkedIndices * [ILM] Add PR review fixes * [ILM] Add PR review fixes (filter fn) * [ILM] Fix i18n errors Co-authored-by: Elastic Machine <[email protected]>
* [ILM] TS conversion of policies table * [ILM] Fix type check errors * [ILM] Fix i18n check errors * [ILM] Add types to linkedIndices * [ILM] Add PR review fixes * [ILM] Add PR review fixes (filter fn) * [ILM] Fix i18n errors Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Summary
This PR converts Policy table to TypeScript as an ongoing effort of converting the ILM plugin #74271. A more verbose loading indicator was added and a button to retry loading policies after failure.
Screenshot