[Composable template] Demo and PR review fixes

[sebelga]

In this PR I've addressed all the concerns during the demo regarding the creation/edition flow of composable index templates. I've also addressed the PR review changes from @alisonelizabeth and @jrodewig in #70814

Changes

• In "Logistics" step: hide the _meta parameter behind a toggle (bb56efa)
• Don't send empty objects inside the template parameter of the payload (f8b0425)

It is important to not send a template object with empty content like so

template: {
  settings: {},
  aliases: {}
}

I've updated the code to strip those out. Now, if you navigate to the "Index settings" tab, the "Mappings" tab or the "Aliases" tab and don't make any changes, you should be able to preview the request in the "Review" step and see that no "template" is provided

Note there is still an outstanding issue that we will need to address at the form lib level: if you navigate to the "Advanced options" of the mappings editor, and then you go check the request in the "Review" step, all the default values for the mappings will be present.

@alisonelizabeth You might want to copy what I did here in the component template form wizard. Then when you get to the Review step, if there is no "template" in the body, show a red callout and not allow saving the component.

• Order the component templates by name in the "Component templates" step (913cfeb)

• Add the missing "Data stream" toggle field in the "Logistics" step (a4bf5e4)

• In the details panel "Summary" tab, put the "_meta" using the full width of the flyout (bd9b0c2)

• Add badges for "Managed", "Cloud-managed" and "System" index template inside the table. And also add those in the "View" dropdown menu. I also added the badge next to the name inside the details flyout. (a8d2727, 6f1bef5)

• Made the copy text changes. (faf6426)

[elasticmachine]

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

[jloleysens]

This is looking good @sebelga , great work!

Remarks on UI/UX:

Borders

I noticed that the mappings editor does not have a border top, I am guessing this is intentional but it did look a little weird to me:

Similarly for the global view selector (this time the right border looks like it is "missing")

Drag 'n drop composable templates

Not a massive issue at all, but thought dragging a component template outside of the droppable area should not cut it off, at the moment it does (not really visible in screenshot, just adding to confirm what I am referring to)

Fuzzy match bug

I noticed adding a regex char into the search component templates bar caused the UI to hard crash with this error (provided input logs*):

Uncaught SyntaxError: Invalid regular expression: /.*l.*o.*g.*s.**.*/: Nothing to repeat
    at new RegExp (<anonymous>)
    at fuzzyMatch (component_templates.tsx:17)
    at component_templates.tsx:82
    at Array.filter (<anonymous>)
    at component_templates.tsx:65
    at updateMemo (react-dom.development.js:16854)
    at Object.useMemo (react-dom.development.js:17334)
    at useMemo (react.development.js:1643)
    at ComponentTemplates (component_templates.tsx:60)
    at renderWithHooks (react-dom.development.js:16260)

We can reuse kibana/x-pack/plugins/index_management/public/application/components/mappings_editor/lib/search_fields.tsx escapeRegExp function to address this. Perhaps we can move this to es-ui-shared (not necessary for now though).

Navigation

Unrelated to this PR, but just wanted to field the idea here. Clicking between composable templates and index templates (or any other tab in Index Management) feels like a bit of a chore. I think we could help users out by remembering what tab they were last on in index management and going back there, only when they have actually entered the app and started click between tabs (i.e., do not persist in localStorage that could be annyoing in a different way). Let me know what you think!

[jloleysens]

Not sure this function name and the logic match up for me. I would expect objHasProperties to return false for when the value passed in is undefined.

Object.keys(obj).length === 0 ? false : true can be written as Object.keys(obj).length !== 0

[sebelga]

I would expect objHasProperties to return false for when the value passed in is undefined.

🤔 hmmm isn't it what this code does?

[cjcenizal]

Maybe the code could be made a bit more verbose to make its behavior more obvious?

const objHasProperties = (obj?: Record<string, any>): boolean => {
  if (obj === undefined) {
    return false;
  }

  if (Object.keys(obj).length === 0) {
    return false;
  }

  return true;
}

[jloleysens]

@sebelga I see that I misread the precedence of the evaluation.

I thought obj === undefined would short-circuit Object.keys(obj).length === 0... check. That is also why I recommended changing it to Object.keys(obj).length !== 0. If prettier allows, adding () around the predicate could also help.

[jloleysens]

Actually to clarify; I am happy with return obj !== undefined && Object.keys(obj).length !== 0. Or CJ's suggestion :)

[sebelga]

What is wrong with the "or" operator? 😄

const objHasProperties = (obj?: Record<string, any>): boolean => {
  if (obj === undefined || Object.keys(obj).length === 0) {
    return false;
  }

  return true;
}

And then, I decided to use a ternary to remove the if clause.

I thought obj === undefined would short-circuit Object.keys(obj).length === 0...

I does short-circuit so it does not execute Object.keys(...) on an undefined value

[jloleysens]

I does short-circuit so it does not execute Object.keys(...) on an undefined value

I see my comment was not very clear, I meant short circuit the whole -> Object.keys(obj).length === 0 ? false : true part of the line (which is wrong). This is a case where the JS grammar seems ambiguous to me.

I'll leave it up to you!

[sebelga]

Thanks for the review @jloleysens !

For the borders, it is all EUI so I prefer to leave it like this for the moment. We could revisit and see if this is inconsistent with the rest of Kibana.

dragging a component template outside of the droppable area should not cut it off

Good call. 👍 This is also EUI that adds a mask-image for the scrollable area. I've overriden it so the items are not cut off while dragging.

I noticed adding a regex char into the search component templates bar caused the UI to hard crash with this error

Great catch! I've copied the sanitized function over and fixed it 👍

Unrelated to this PR, but just wanted to field the idea here. Clicking between composable templates and index templates (or any other tab in Index Management) feels like a bit of a chore. I think we could help users out by remembering what tab they were last on in index management and going back there, only when they have actually entered the app and started click between tabs (i.e., do not persist in localStorage that could be annyoing in a different way). Let me know what you think!

This is a great point. There has been multiple times I wish we had a simple preferenceService.save({ some: 'value' }) that would abstract for us if the user has security turned on (and thus save to ES) or not, with a localStorage fallback. I know there is some issue/work started for that but I don't know the status. We could bring this up at one of our sync, I think this feature would improve a lot our UX in multiple places.

Can you have another look at the PR and give it a 👍 if everything is good?

[jloleysens]

Thanks for making those changes @sebelga !

Tested again locally and everything looks to be in order 🍻

[cjcenizal]

"Wheter" is misspelled, and I don't think we need a line break between these two sentences. I'd also suggest rewording the text as an action but would like to hear @jrodewig's input:

Create a data stream instead of an index. Learn more about data streams.

[cjcenizal]

Can we use documentationService.getDataStreamsDocumentationLink() for this href instead? And I think we need to add the external prop to this link.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: bd1c949

Build metrics

✅ unchanged

History

• 💔 Build #60299 failed 8215718
• 💔 Build #60244 failed 67c2677
• 💔 Build #60227 failed 9c3e329
• 💔 Build #59695 failed c530093

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

[alisonelizabeth]

@cjcenizal I'm going to go ahead and merge this PR since CI is finally green 😄 . I'll address your comments in a separate PR.