Migrate Index Management to new solutions nav by cjcenizal · Pull Request #101548 · elastic/kibana

cjcenizal · 2021-06-07T23:23:33Z

Partially addresses #100748

Notable changes:

I made the shared PageError component's error prop optional so we could use it to surface errors like missing permissions, which don't report an ES error.
Added a shared PageLoading component.
The index_table component tests are very brittle and I had to change them slightly to get them passing. Added comments to the code to mark brittle areas.
The "get index templates" API route didn't report errors, so I had to add that in.
Added actions to the index templates list empty prompt.

Screenshots

Indices

Data streams

Index templates

List

Wizard

Component templates

List

Wizard

cjcenizal · 2021-06-17T22:57:59Z

...plication/components/component_templates/component_template_list/component_template_list.tsx

I know it looks weird but this is deliberate. I think we need to go back and rearchitect our apps later for consistent patterns both across apps and even within each app.

I've never been a big fan of this content var declared on top and assigned down.

Can we write this slightly differently, with simply an early return?

if (isLoading) { return ( <PageLoading data-test-subj="sectionLoading"> <FormattedMessage id="xpack.idxMgmt.home.componentTemplates.list.loadingMessage" defaultMessage="Loading component templates…" /> </PageLoading> ); } let content: React.ReactNode; if (data?.length) { ...

We could but it would require duplicating the wrapping <div className={APP_WRAPPER_CLASS} data-test-subj="componentTemplateList"> element in several places. I think we can find a more elegant solution later when we rearchitect our apps to adhere to a consistent pattern, so I'm going to defer this for now.

On second reading it sounds like you're advocating for moving the content var so it isn't declared before the early return, which SGTM.

It all goes to down to preferences but I never have to use this pattern of declaring an empty content and assign it in multiple place to finally render it down. I much prefer early return pattern

const SomeComponent = () => { if (this) { return <MyComp />; } if (that) { return <MyOtherComp />; } const renderMainContent = () => { if (someFlag) { return <div>Early return</div>; } return <div>Finally</div>; } // otherwise return <div className="someClassWeWantEverywhere">{renderMainContent()}</div> }

Ah, I see what you're saying. Yes I prefer that, too. We'll apply this in the future when we rearchitect our apps to adhere to a consistent pattern.

cjcenizal · 2021-06-17T23:04:50Z

...ck/plugins/index_management/public/application/sections/home/template_list/template_list.tsx

This empty prompt didn't originally offer the user any actions, so I added those and updated the copy to mirror our other empty prompts.

cjcenizal · 2021-06-17T23:08:38Z

x-pack/plugins/index_management/public/application/services/use_request.ts

This allows us to pass API errors to PageError without a TS error.

elasticmachine · 2021-06-17T23:09:54Z

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

cjcenizal · 2021-06-17T23:18:56Z

...plication/components/component_templates/component_template_list/component_template_list.tsx

I removed this state because it didn't surface the underlying error, offered a "Try again" button which I don't think will be helpful in the typical error scenario, and was inconsistent with the other error states in this PR.

* Add PageLoading component to es_ui_shared.

* Refactor index table component tests.

* Add missing error reporting to get all templates API route handler.

…_shared SectionLoading elsewhere.

sebelga

Great job @cjcenizal ! Tested locally and works as expected. I left a few non blocking comments in the code.

sebelga · 2021-06-22T09:10:44Z

src/plugins/es_ui_shared/public/components/page_loading/page_loading.tsx

+import React from 'react';
+import { EuiEmptyPrompt, EuiLoadingSpinner, EuiText, EuiPageContent } from '@elastic/eui';
+
+interface Props {


nit: you don't need to declare this interface. React.FunctionComponent already declare the children prop for us.

Thanks! Fixed.

sebelga · 2021-06-22T09:18:33Z

x-pack/plugins/index_management/__jest__/components/index_table.test.js

+  // but it looks like we're aware of how many Redux actions are dispatched in response to user interaction,
+  // so we "time" our assertion based on how many Redux actions we observe. This is brittle because it
+  // depends upon how our UI is architected, which will affect how many actions are dispatched.
+  // Expect this to break when we rearchitect the UI.


I'd rather put: "Expect to completely rewrite those tests when we rearchitect the UI"! 😄 This would be one of our tech debt. Actually I'd first rewrite the tests and follow our CIT best practices (no testing of implementation details!) and only after I would rearchitect the UI.

Haha yep! I'm just marking the landmine with this comment, not prescribing how it should be addressed. I think an issue is a better place for that (more discoverable and collaborative). I'm going to raise this as a tech debt item we should address.

sebelga · 2021-06-22T09:19:58Z

x-pack/plugins/index_management/__jest__/components/index_table.test.js

  });
-  test('should show more when per page value is increased', () => {
+
+  test('should show more when per page value is increased', async () => {


Are we testing the EUI table component here?

The indices table is implemented using individual EuiTable, EuiTablePagination, EuiTableRowCell, etc components. So unfortunately, no, this isn't testing EUI. It's testing our table implementation which EUI now provides an abstraction over with EuiBasicTable. More tech debt! 🎉

sebelga · 2021-06-22T09:23:14Z

...n/components/component_templates/__jest__/client_integration/component_template_list.test.ts

      expect(exists('componentTemplatesLoadError')).toBe(true);
      expect(find('componentTemplatesLoadError').text()).toContain(
-        'Unable to load component templates. Try again.'
+        'Error loading component templatesInternal server error'


It seems that a space and a dot is missing after "templates".

Nice spot! This is not a bug though, just an oddity due to the way the DOM API provides text values of child elements by concatenating each child element's text value without any delimiter. So it's smashing together the title element "Error loading component templates" with the description element "Internal server error". Will add a comment to clarify.

sebelga · 2021-06-22T09:27:53Z

...plication/components/component_templates/component_template_list/component_template_list.tsx

I've never been a big fan of this content var declared on top and assigned down.

Can we write this slightly differently, with simply an early return?

if (isLoading) { return ( <PageLoading data-test-subj="sectionLoading"> <FormattedMessage id="xpack.idxMgmt.home.componentTemplates.list.loadingMessage" defaultMessage="Loading component templates…" /> </PageLoading> ); } let content: React.ReactNode; if (data?.length) { ...

sebelga · 2021-06-22T09:37:26Z

x-pack/plugins/index_management/public/application/components/template_form/template_form.tsx

    <>
      {/* Form header */}
-      {title}
+      <EuiPageHeader pageTitle={<span data-test-subj="pageTitle">{title}</span>} bottomBorder />


Should we ask the EUI team to add a generic data-test-subj for us so we don't have to provide everywhere a <span> for the title? We should only have one page header per page right?

Good idea. They already have a generic data-test-subj, but we're excluding the work of migrating our code and tests over to it from the scope of this work due to time pressure.

sebelga · 2021-06-22T09:38:50Z

...gins/index_management/public/application/sections/home/data_stream_list/data_stream_list.tsx

  if (isLoading) {
    content = (
-      <SectionLoading>
+      <PageLoading>


Same here. It seems we did some copy/paste of this pattern 😊

I'm not sure what you're suggesting here, can you clarify? If it's the same comment as #101548 (comment) then I'll defer to this later when we rearchitect for a consistent pattern across apps.

Yes it was the same comment about early return. It's OK to leave it as is, goes down to preferences. 👍

sebelga · 2021-06-22T09:41:03Z

...gins/index_management/public/application/sections/home/index_list/index_table/index_table.js

+    if (!hasContent) {
+      const renderNoContent = () => {
+        if (indicesLoading) {
+          return (


Returning early here again would make the code easier to read.

Do you mean converting else if to if? From my reading of the code, it looks like it's already returning early.

Yes, once you have an early return there is no need of else if. Just code styling thing, nothing important.

if (something) { return true; } return false; // instead of if (something) { return true; } else { return false; }

sebelga · 2021-06-22T09:43:08Z

...ck/plugins/index_management/public/application/sections/home/template_list/template_list.tsx

-    if (isLoading) {
-      return (
-        <SectionLoading>
+  // Track component loaded


nit: wouldn't it be "templates" loaded?

Hmm this is pre-existing so I can't be certain, but it looks like this code will execute when the component mounts, regardless of whether the templates are loading, loaded, or failed to load. So I have to assume that by "loaded" the original author meant "mounted". Fixed.

No I meant that the tracking. UIM_TEMPLATE_LIST_LOAD is for "index template loaded" not "component templates loaded" 😊

sebelga · 2021-06-22T09:44:02Z

...ck/plugins/index_management/public/application/sections/home/template_list/template_list.tsx

kibanamachine · 2021-06-23T00:09:07Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`esUiShared`	153	155	+2
`indexManagement`	518	514	-4
total			-2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`esUiShared`	88	90	+2

Async chunks

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

id	before	after	diff
`indexManagement`	1.3MB	1.3MB	-2.0KB

Page load bundle

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

id	before	after	diff
`esUiShared`	191.8KB	192.5KB	+770.0B

Unknown metric groups

API count

id	before	after	diff
`esUiShared`	90	92	+2

History

💚 Build #132933 succeeded a4f510a
💔 Build #132747 failed fe9b3464a29c2386f38ec80d0985deff413d6976
💚 Build #132295 succeeded 5c26e4973cb8565fcf9ea073727a7091d8c28f06
💚 Build #131923 succeeded a13c70c1cddd044faa58d88451d103e71eb96f56
💔 Build #131566 failed fc128ce5b1caf83f3bd77417a1876e283a97f453

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

* Migrate index template and component template wizard pages to new nav. * Convert index templates and component templates pages to new nav. * Convert indices and data streams pages to new nav. * Add PageLoading component to es_ui_shared. * Refactor index table component tests. * Add missing error reporting to get all templates API route handler. # Conflicts: # x-pack/plugins/index_management/server/routes/api/templates/register_get_routes.ts

cjcenizal added the Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// label Jun 7, 2021

cjcenizal mentioned this pull request Jun 7, 2021

Convert Stack Management apps to new Management nav structure #100748

Closed

10 tasks

cjcenizal force-pushed the index-management/migrate-nav branch 2 times, most recently from 88e1c24 to d2a446f Compare June 8, 2021 23:06

alisonelizabeth mentioned this pull request Jun 10, 2021

[Snapshot + Restore] Migrate to new page layout structure #101811

Merged

cjcenizal force-pushed the index-management/migrate-nav branch 4 times, most recently from fc128ce to a13c70c Compare June 16, 2021 23:23

cjcenizal added chore Feature:Index Management Index and index templates UI v7.14.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Jun 17, 2021

cjcenizal force-pushed the index-management/migrate-nav branch from a13c70c to ece3512 Compare June 17, 2021 21:31

cjcenizal commented Jun 17, 2021

View reviewed changes

cjcenizal marked this pull request as ready for review June 17, 2021 23:09

cjcenizal requested a review from a team as a code owner June 17, 2021 23:09

cjcenizal requested review from alisonelizabeth and sebelga and removed request for alisonelizabeth June 17, 2021 23:10

cjcenizal commented Jun 17, 2021

View reviewed changes

cjcenizal mentioned this pull request Jun 21, 2021

[CCR] Migrate to new page layout structure #102507

Merged

cjcenizal force-pushed the index-management/migrate-nav branch from 5c26e49 to fe9b346 Compare June 21, 2021 15:22

cjcenizal added 5 commits June 21, 2021 14:30

Migrate index template and component template wizard pages to new nav.

f10275f

Convert index templates and component templates pages to new nav.

8e05b21

* Add PageLoading component to es_ui_shared.

Convert indices and data streams pages to new nav.

1f36a02

* Refactor index table component tests.

Center error states in index templates and component templates lists.

aca08de

* Add missing error reporting to get all templates API route handler.

Use PageLoading instead of SectionLoading for lists. Migrate to es_ui…

e98a025

…_shared SectionLoading elsewhere.

Remove inline styles from template_list.

a4f510a

cjcenizal force-pushed the index-management/migrate-nav branch from fe9b346 to a4f510a Compare June 21, 2021 21:53

sebelga approved these changes Jun 22, 2021

View reviewed changes

cjcenizal added 2 commits June 22, 2021 14:00

Merge branch 'master' into index-management/migrate-nav

3a6661d

Apply Seb's suggestions.

db94a22

cjcenizal merged commit 5df858a into elastic:master Jun 23, 2021

cjcenizal deleted the index-management/migrate-nav branch June 23, 2021 00:16

cjcenizal mentioned this pull request Jun 23, 2021

[7.x] Migrate Index Management to new solutions nav (#101548) #103029

Merged

Conversation

cjcenizal commented Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Indices

Data streams

Index templates

List

Wizard

Component templates

List

Wizard

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jun 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebelga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

cjcenizal commented Jun 7, 2021 •

edited

Loading

cjcenizal Jun 22, 2021 •

edited

Loading

cjcenizal Jun 22, 2021 •

edited

Loading

cjcenizal Jun 22, 2021 •

edited

Loading

cjcenizal Jun 22, 2021 •

edited

Loading