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

Create a new menu for observability links #54847

Merged
merged 19 commits into from
Jan 28, 2020

Conversation

phillipb
Copy link
Contributor

@phillipb phillipb commented Jan 14, 2020

Summary

Created a new component for the observability action menu, and started using it on the metrics inventory screen. Fixes #53282.

TODO: Move the component to a central area, so that it can be used by all observability teams.

Screen Shot 2020-01-14 at 5 05 51 PM

Screen Shot 2020-01-14 at 5 10 19 PM

Screen Shot 2020-01-14 at 5 08 43 PM

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

@phillipb phillipb added Feature:Metrics UI Metrics UI feature v8.0.0 Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.7.0 labels Jan 14, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@bmorelli25
Copy link
Member

bmorelli25 commented Jan 14, 2020

Not sure if it's within the scope of this PR, but there were talks of potentially also showing the value of host.ip, container.id, etc. in these action menus. See this somewhat related issue in the uptime repo for the conversation elastic/uptime#128. Not sure what was ultimately decided on, so tagging @katrin-freihofner, since I think she's been spearheading this.

Edit: Also, in your description, this should probably close #53590, not #53282

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

Visually this looks pretty awesome ✨

How about we use an API for the menu components that favors composition over configuration such that a consumer has more flexibility? It could look something like

<ActionMenuPopover
  closePopover={...}
  isOpen={...}
>
  <ActionMenuSection>
    <ActionMenuSectionTitle>
      <FormatMessage ... />
    </ActionMenuSectionTitle>
    <ActionMenuSectionSubtitle>
      <FormatMessage ... />
    </ActionMenuSectionSubtitle>
    <ActionMenuLinks>
      <ActionMenuLink href="">
        <FormatMessage ... />
      </ActionMenuLink>
      <ActionMenuLink href="">
        <FormatMessage ... />
      </ActionMenuLink>
    </ActionMenuLinks>
  </ActionMenuSection>
  <ActionMenuSectionDivider />
  <ActionMenuSection>
    <ActionMenuLinks>
      <ActionMenuLink href="">
        <FormatMessage ... />
      </ActionMenuLink>
      <ActionMenuLink href="">
        <FormatMessage ... />
      </ActionMenuLink>
    </ActionMenuLinks>
  </ActionMenuSection>
</ActionMenuPopover>

The <ActionMenu*> components would map to the correctly parameterized EUI components and take their content as children. That way we would make sure the popovers are styled consistently while remaining open for extension and adaptation to specific needs (such as embedding a visualizations).

@phillipb
Copy link
Contributor Author

@weltenwort thanks for the feedback! A couple of thoughts on that approach.

  1. It looks like the current design specifies the use of a EuiListGroup (https://elastic.github.io/eui/#/display/list-group) for the links to increase clickability, that api is a config based api. It could be difficult to make a composition based api that maps to that.

  2. I think if we want that level of flexibility, there's not much that's shared. There's no logic in the component, in fact, there aren't even click events. It'd probably be just as easy if everyone just used the built-in EUI components themselves.

Another approach could be to make some of the props either a string | ReactNode. I could see that working pretty easily for the title and subtitle props. I think if you want to customize the layout of the menu though, that it probably makes sense to just create your own component.

@phillipb
Copy link
Contributor Author

One second thought, there could be some semantic benefit there. It's probably easier to think ActionMenuTitle, ActionMenuSubtitle than to remember xs h5 etc. So I resend the second thought about sharing. However, I still wonder about point 1.

@weltenwort
Copy link
Member

that api is a config based api

The example JS code on the EUI page suggests the items can also be passed as children 🤔

        <EuiListGroup flush={flushWidth} bordered={showBorder}>
          <EuiListGroupItem onClick={handleOnClick} label="First item" />

          <EuiListGroupItem onClick={handleOnClick} label="Second item" />

          <EuiListGroupItem
            onClick={handleOnClick}
            label="Third item"
            isActive
          />

          <EuiListGroupItem
            onClick={handleOnClick}
            label="Fourth item"
            isDisabled
          />
        </EuiListGroup>

@katrin-freihofner
Copy link
Contributor

@phillipb that looks great 🤩.
As @bmorelli25 mentioned, we would like to get the host.ip, container.id, pod.id...in the subheadline. This is an example of the text for the uptime menu that was mentioned:
Screenshot 2020-01-15 at 18 57 06

Are you also planning to do the shared component?

Thank you for your great work!

@phillipb
Copy link
Contributor Author

Are you also planning to do the shared component?

@katrin-freihofner: yep. I just need to figure out where that should be. @weltenwort any ideas on the right place to put this?

@weltenwort
Copy link
Member

any ideas on the right place to put this?

Good question 🤔 there used to be an observability plugin intended for sharing such components, but I can't seem to find it anymore. @jasonrhodes, any idea what happened to that?

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

This is really good 👏 I just left a few questions below.

@phillipb
Copy link
Contributor Author

@katrin-freihofner with the addition of aws metrics, we now support more than just host, container and pod. What id/ip should we show for AWS EC2, RDS, S3, and SQS?

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

The code looks good 👍

While looking at action menus with long non-breakable descriptions (like a container id) I noticed that the layout breaks a bit:

grafik

@phillipb
Copy link
Contributor Author

phillipb commented Jan 23, 2020

While looking at action menus with long non-breakable descriptions (like a container id) I noticed that the layout breaks a bit:

grafik

Maybe a max-width on the popup + word-wrap? Or should I just truncate with an ellipsis? @katrin-freihofner any preference?

@katrin-freihofner
Copy link
Contributor

I think to truncate and ellipsis is fine.

@simianhacker
Copy link
Member

@phillipb @katrin-freihofner We should also add a tooltip with the full value.

@jasonrhodes
Copy link
Member

any ideas on the right place to put this?

Good question 🤔 there used to be an observability plugin intended for sharing such components, but I can't seem to find it anymore. @jasonrhodes, any idea what happened to that?

🤔 how strange, it appears to have disappeared! @phillipb, let's set up a small plugin in the new platform space, called "observability", and then we can export this component from the public/index file of that plugin. You can model it after the infra plugin that's in x-pack/plugins right now, but it obviously doesn't need to be as fully featured. I'm happy to help.

@phillipb phillipb marked this pull request as ready for review January 24, 2020 21:00
@phillipb phillipb requested a review from a team as a code owner January 24, 2020 21:00
@elasticmachine
Copy link
Contributor

user doesn't have permission to update head repository

@phillipb
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

user doesn't have permission to update head repository

Copy link
Member

@simianhacker simianhacker left a comment

Choose a reason for hiding this comment

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

LGTM

@jasonrhodes
Copy link
Member

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

user doesn't have permission to update head repository

@jasonrhodes
Copy link
Member

@phillipb can you check the Allow edits from maintainers checkbox on this PR? That will allow elasticmachine to update the branch...

@phillipb
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@phillipb phillipb merged commit 1ec7ee7 into elastic:master Jan 28, 2020
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 28, 2020
…ve-out-legacy

* 'master' of github.com:elastic/kibana: (187 commits)
  [ML] Reseting categorization validation if category field is cleared (elastic#56029)
  [SIEM] Fields browser readable (elastic#56000)
  [docs] Remove unused callout (elastic#56032)
  Refactor saved object management registry usage (elastic#54155)
  [SIEM][Detection Engine] critical blocker, updates the pre-packaged rules, removes dead ones, adds license file (elastic#56090)
  Fix failing snapshot artifact tests when using env var (elastic#56063)
  Fix Github PR comment formatting (elastic#56078)
  [Maps] fix join metric field selection bugs (elastic#56044)
  Create a new menu for observability links (elastic#54847)
  [SIEM] [Detection Engine] Fixes histogram intervals  (elastic#55969)
  make test less flaky by retrying if list is re-rendered (elastic#55949)
  Remove matrix build support (elastic#54202)
  Add animation to service map layout (elastic#56042)
  [Canvas] Remove Angular and unnecessary reporting config from Canvas (elastic#54050)
  [Uptime] Simplify snapshot max to Infinity (elastic#55931)
  [Uptime] Reintroduce a column for url (elastic#55451)
  Cleanup action task params objects after successful execution (elastic#55227)
  [CI] Retry flaky tests (elastic#53961)
  Expose NP FieldFormats service to server side (elastic#55419)
  [Endpoint] EMT-65: make endpoint data types common, restructure (elastic#54772)
  ...

# Conflicts:
#	src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/__snapshots__/split_panel.test.tsx.snap
#	src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/containers/panel.tsx
#	src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/context.tsx
#	src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/index.ts
#	src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/split_panel.test.tsx
#	src/legacy/ui/public/vis/editors/default/default_editor.tsx
#	src/plugins/console/public/application/components/split_panel/__snapshots__/split_panel.test.tsx.snap
#	src/plugins/console/public/application/components/split_panel/components/resizer.tsx
#	src/plugins/console/public/application/components/split_panel/containers/panel.tsx
#	src/plugins/console/public/application/components/split_panel/containers/panel_container.tsx
#	src/plugins/console/public/application/components/split_panel/context.tsx
#	src/plugins/console/public/application/components/split_panel/index.ts
#	src/plugins/console/public/application/components/split_panel/registry.ts
#	src/plugins/console/public/application/components/split_panel/split_panel.test.tsx
#	src/plugins/kibana_react/public/split_panel/__snapshots__/split_panel.test.tsx.snap
#	src/plugins/kibana_react/public/split_panel/containers/panel.tsx
#	src/plugins/kibana_react/public/split_panel/context.tsx
#	src/plugins/kibana_react/public/split_panel/index.ts
#	src/plugins/kibana_react/public/split_panel/split_panel.test.tsx
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 28, 2020
* master: (77 commits)
  [ML] Reseting categorization validation if category field is cleared (elastic#56029)
  [SIEM] Fields browser readable (elastic#56000)
  [docs] Remove unused callout (elastic#56032)
  Refactor saved object management registry usage (elastic#54155)
  [SIEM][Detection Engine] critical blocker, updates the pre-packaged rules, removes dead ones, adds license file (elastic#56090)
  Fix failing snapshot artifact tests when using env var (elastic#56063)
  Fix Github PR comment formatting (elastic#56078)
  [Maps] fix join metric field selection bugs (elastic#56044)
  Create a new menu for observability links (elastic#54847)
  [SIEM] [Detection Engine] Fixes histogram intervals  (elastic#55969)
  make test less flaky by retrying if list is re-rendered (elastic#55949)
  Remove matrix build support (elastic#54202)
  Add animation to service map layout (elastic#56042)
  [Canvas] Remove Angular and unnecessary reporting config from Canvas (elastic#54050)
  [Uptime] Simplify snapshot max to Infinity (elastic#55931)
  [Uptime] Reintroduce a column for url (elastic#55451)
  Cleanup action task params objects after successful execution (elastic#55227)
  [CI] Retry flaky tests (elastic#53961)
  Expose NP FieldFormats service to server side (elastic#55419)
  [Endpoint] EMT-65: make endpoint data types common, restructure (elastic#54772)
  ...
phillipb added a commit to phillipb/kibana that referenced this pull request Jan 28, 2020
* Create a new menu for observability links. Use it on inentory page.

* Change the order of props for clarity

* Fix default message

* Composition over configuration

* Show ids and ips. PR feedback.

* Don't wrap subtitle. Use fields in inventory model for name

* Tooltip was becoming hacky. Keep it simple and wrap the id.

* Create observability plugin. Add action menu to it.

* Fix path

* Satisfy linter and fix test

* Please the linter

* Update translastions

* Update test for disabled links

* Update more tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
phillipb added a commit that referenced this pull request Jan 28, 2020
* Create a new menu for observability links. Use it on inentory page.

* Change the order of props for clarity

* Fix default message

* Composition over configuration

* Show ids and ips. PR feedback.

* Don't wrap subtitle. Use fields in inventory model for name

* Tooltip was becoming hacky. Keep it simple and wrap the id.

* Create observability plugin. Add action menu to it.

* Fix path

* Satisfy linter and fix test

* Please the linter

* Update translastions

* Update test for disabled links

* Update more tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Metrics UI Metrics UI feature release_note:enhancement Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Observability UI] actions menu update
8 participants