Skip to content
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

[Component templates] Address feedback #70912

Merged

Conversation

alisonelizabeth
Copy link
Contributor

@alisonelizabeth alisonelizabeth commented Jul 7, 2020

This PR addresses some of the feedback received on the component templates UI:

  • Added description under the "Component templates" tab
  • Added "Managed" badge with table filter
  • Added a call to action to create/edit an index template to the details panel when the template is not in use. Note: I made this an optional prop, as I don't think it makes sense to show this when the details panel is displayed in the "Components" step in the index templates wizard.
  • Addressed copy review feedback
  • Do not send empty mappings/settings/aliases objects (followed similar implementation as in [Composable template] Demo and PR review fixes #71065 (comment)).

Screenshots

Screen Shot 2020-07-07 at 12 14 32 PM

Screen Shot 2020-07-07 at 8 46 01 AM

Screen Shot 2020-07-07 at 8 46 07 AM

@alisonelizabeth alisonelizabeth added Feature:Index Management Index and index templates UI v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jul 7, 2020
@alisonelizabeth alisonelizabeth marked this pull request as ready for review July 7, 2020 13:16
@alisonelizabeth alisonelizabeth requested a review from a team as a code owner July 7, 2020 13:16
@elasticmachine
Copy link
Contributor

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

@alisonelizabeth alisonelizabeth requested a review from sebelga July 7, 2020 13:17
@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @alisonelizabeth 👏🏻! I tested creating a component template and an index template (and editing both) and every was working as expected.

Left non-blocker comments.


{componentTemplateDetails?._kbnMeta.isManaged ? (
<EuiFlexItem grow={false}>
{' '}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this whitespace intended to be included? Perhaps &npsb; would be better if so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is intentional. I'm going to leave as is for now, but will consider changing in a follow-up PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI the empty space that Alison used will allow a line break if the container forces the text to wrap, but a non-breaking space won’t allow this. In most situations I think we want the former behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out @cjcenizal!

Copy link
Contributor

@sebelga sebelga Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI the empty space that Alison used will allow a line break if the container forces the text to wrap, but a non-breaking space won’t allow this. In most situations I think we want the former behavior.

@cjcenizal Not sure I understand correctly. Are we talking about 1 space difference (probably a few pixels) that would not wrap? Ex: hello world (there is a space before the "h"): where would this not wrap after "hello"?

Copy link
Contributor

@cjcenizal cjcenizal Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure if this answers your question but I was just pointing out that &nbsp; !== ‘ ‘. The former won’t allow a line break on the space (“hello world” will overflow its container) while the latter will (“hello world” will break into two lines consisting of “hello” and “world”).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is strange, I would need to try it out 😊 It would make sense if we were talking about hello&nbsp;world but here we are adding the   before the 2 words.

const cleanupComponentTemplateObject = (componentTemplate: ComponentTemplateDeserialized) => {
const outputTemplate = { ...componentTemplate };

if (outputTemplate.template.settings === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could there ever be a case where "settings", "mappings" or "aliases" is an empty object at this point? If so we should probably also delete that from the output template.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@alisonelizabeth alisonelizabeth merged commit 105afbc into elastic:master Jul 13, 2020
@alisonelizabeth alisonelizabeth deleted the component_templates/feedback branch July 13, 2020 16:10
alisonelizabeth added a commit to alisonelizabeth/kibana that referenced this pull request Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Index Management Index and index templates UI 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 v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants