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

[ML] Explain log rate spikes: Fix data view title. #137053

Merged
merged 5 commits into from
Jul 26, 2022

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Jul 25, 2022

Summary

Part of #136265.

  • Use dataView.getName() instead of dataView.title.
  • Adds missing scss brought over from Data Visualizer to improve positioning of long Data View names.

Before:

image

After:

image

Checklist

@walterra walterra added bug Fixes for quality problems that affect the customer experience :ml release_note:skip Skip the PR/issue when compiling release notes v8.4.0 labels Jul 25, 2022
@walterra walterra requested a review from a team as a code owner July 25, 2022 10:38
@walterra walterra self-assigned this Jul 25, 2022
@walterra walterra requested a review from a team as a code owner July 25, 2022 10:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@sophiec20
Copy link
Contributor

Much improved to use full width. Index names can be long, so we should plan for this.
Does it need to be so bold and so big?
(Happy to leave as is if it follows design guidelines, and I don't need to be on a review, but just asking the question...:shrug:)

@walterra
Copy link
Contributor Author

Agree parts of the layout are worth revisiting. For now we keep it in line with Data Visualizer and reuse code from there too. We plan to consolidate some of the components, when we do that we can have a look at the layout too.

<EuiTitle size={'s'}>
<h2>{dataView.title}</h2>
<h2>{dataView.getName()}</h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we add an EuiSpacer underneath?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 2f404a8.

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

Only edit I see is add some spacing between the title and components below it. Can be either a spacer or in the scss I imagine.

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

Other than Dima's comment about adding the spacer - LGTM ⚡

@walterra
Copy link
Contributor Author

Fixed the title spacing. Because this layout variant is part of responsive rules a spacer wasn't suitable because it would break the wider layout's vertical alignment, so added some padding to the title element.

image

Ready for another look @mdefazio.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
aiops 354 359 +5

Async chunks

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

id before after diff
aiops 467.0KB 468.7KB +1.7KB
dataVisualizer 561.5KB 561.5KB +28.0B
total +1.7KB

History

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

cc @walterra

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

LGTM!

@walterra walterra merged commit bd9955a into elastic:main Jul 26, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 26, 2022
@walterra walterra deleted the ml-aiops-data-view-title branch July 26, 2022 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting bug Fixes for quality problems that affect the customer experience :ml release_note:skip Skip the PR/issue when compiling release notes v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants