Migrate ILM to ManagementPageLayout #100838
Conversation
cjcenizal
added
chore
Feature:ILM
Team:Kibana Management
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
cjcenizal
added
backport:skip
| @@ -49,7 +49,7 @@ export class IndexLifecycleManagementPlugin | |||
| id: PLUGIN.ID, | |||
| title: PLUGIN.TITLE, | |||
| order: 2, | |||
| mount: async ({ element, history, setBreadcrumbs }) => { | |||
| mount: async ({ element, history, setBreadcrumbs, managementPageLayout }) => { | |||
There was a problem hiding this comment.
Step 1: Declare the injected managementPageLayout within the plugin's start lifecycle method. Note that we have to pass it into the context (wherever the plugin happens to define it), as seen below on line 80.
| cloud?: CloudSetup | ||
| ): UnmountCallback => { | ||
| render( | ||
| <I18nContext> | ||
| <KibanaContextProvider services={{ cloud, breadcrumbService, license }}> | ||
| <KibanaContextProvider services={{ cloud, breadcrumbService, license, managementPageLayout }}> |
There was a problem hiding this comment.
Step 2: Use KibanaContextProvider to provide it to the app using React Context. Note that we have to update our types in the types.ts file to support the change to this interface.
| iconType="managementApp" | ||
| title={ | ||
| <h1> | ||
| <ManagementPageLayout isEmptyState={true}> |
There was a problem hiding this comment.
Step 3: For every rendered view, use the useKibana hook to access managementPageLayout, cast it to ManagementPageLayout so it looks like a React component, and wrap the rendered view in it. Here on line 125 we're migrating an empty rendered view. On line 153, below, we're migrating a non-empty rendered view. Note the differences in the deltas.
| services={{ | ||
| breadcrumbService, | ||
| license: licensingMock.createLicense(), | ||
| managementPageLayout: KibanaPageTemplate, |
There was a problem hiding this comment.
Step 4: Update Jest tests to set up testbed by stubbing the injected managementPageLayout with KibanaPageTemplate.
There was a problem hiding this comment.
Nice one! I Tested locally and it all seems to be working a-ok. Only one issue I found is related to the colours of the ManagementPageLayout defined with isEmptyState={true} and the EuiSideNav being the same and giving no clear distinction between the both sections.
| /> | ||
| } | ||
| /> | ||
| <ManagementPageLayout isEmptyState={true}> |
There was a problem hiding this comment.
When using the isEmptyState prop, the content panel ends up with a grey background which is the same the apps navigation has. This makes it look a bit confusing as it seems its all one big section instead of two clearly distinct regions.
We could try following a similar approach to what the observability team did, which is instead of using isEmpyState={true} use template="centeredContent". And while this will give us a more clear division between the navigation and content sections, it has one small drawback which is that we end up with an empty pageHeader (observability seems to have left it there regardless). We could fix that issue though by adding a pageTitle, but I cant find a way to just hide it.
There was a problem hiding this comment.
@sabarasaba Great call! @cchaos This seems like a design problem. Do you have any suggestions on the best way to solve this?
There was a problem hiding this comment.
+++ Yes, don't use the centeredBody when we've got side-nav. I'm currently working on an upgrade to create a pattern similar to the empty state one for loading.
| <EuiButtonEmpty onClick={togglePolicyJsonFlyout} data-test-subj="requestButton"> | ||
| {isShowingPolicyJsonFlyout ? ( | ||
| <FormattedMessage | ||
| id="xpack.indexLifecycleMgmt.editPolicy.hidePolicyJsonButto" |
There was a problem hiding this comment.
Seems this was already here from before, but shouldnt this and line 326 be hidePolicyJsonButton instead?
There was a problem hiding this comment.
Great spot, you're right. I'll fix this in this PR.
| <ManagementPageLayout | ||
| pageHeader={{ | ||
| pageTitle: ( | ||
| <span data-test-subj="policyTitle"> |
There was a problem hiding this comment.
I guess there's not a way to pass a data-test-subj to the page title?
There was a problem hiding this comment.
No it's not supported. Per #100085 (comment) there is a hard-coded data-test-subj for the title created by ManagementPageLayout so we can migrate our code and tests over to use that instead I think. But it's also not a blocker IMO so I've decided not to implement that as part of this migration.
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / jest / Jest Tests.src/plugins/advanced_settings/public/management_app.AdvancedSettings should render unfiltered with query parsing errorStandard OutStack TraceKibana Pipeline / jest / Jest Tests.x-pack/plugins/index_lifecycle_management/__jest__.policy table should show empty state when there are not any policiesStandard OutStack TraceKibana Pipeline / jest / Jest Tests.src/plugins/kibana_react/public/page_template.KibanaPageTemplate render default empty promptStandard OutStack Traceand 17 more failures, only showing the first 3. History
To update your PR or re-run it, just comment with: |
|
Taking a different approach for this |
Summary
Table loading state
Table error state
Table empty state
Table loaded state
Form loading state
Form error state
Form state
Checklist
Delete any items that are not applicable to this PR.
For maintainers