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

Design cleanup of Uptime app #31663

Merged
merged 77 commits into from
Apr 4, 2019
Merged

Design cleanup of Uptime app #31663

merged 77 commits into from
Apr 4, 2019

Conversation

snide
Copy link
Contributor

@snide snide commented Feb 21, 2019

Summary

This does some reworking of the Uptime layouts to get them closer to the rest of Kibana.

Some comments

  • Turned the refresh button the date picker. You don't want this activating without direction I don't think. Also gives people a nice panic REFRESH button.
  • Changed coloring to be more normal. Green / red.
  • Only focus on downtime history in the tables. People likely aren't looking for green here and it can get really busy, they want to scan for the problems.
  • Removed all the paneling
  • Tightened up all the paddings / margins
  • Cleaned up all the tables so they are better sized and formatted.

image

image

EDIT: resolves #29841.
also resolves #31831.

I need the following help

@justinkambic would love some help fixing the following. Couldn't find the code immediately to do this. Also, I couldn't get the snapshots to update. It would just hang.

image

image

Checklist

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

For maintainers

@snide snide added Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Feb 21, 2019
@snide snide requested a review from justinkambic February 21, 2019 06:20
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-design

@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime

@@ -106,15 +106,17 @@ class Application extends React.Component<UptimeAppProps, UptimeAppState> {
let colors: UptimeAppColors;
if (darkMode) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cchaos Do you have a pattern for this kind of thing I could steal. I mostly just recolored what they had here, but it seems like a silly way to do this when the colors vars themselves are the same outside of the theme.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have not run into any theme vars imported via JSON in any of my fixes yet. You could ask SIEM or APM since I'm pretty sure they do. My initial thought would be to set the import path based on the global theme but I'm not exactly sure the way to do this.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@snide
Copy link
Contributor Author

snide commented Feb 21, 2019

Cleaned up the gray coloring of that graph and played with the strokes a bit. For the status check I couldn't figure out why this graph was delivered as an area graph (the others are bars) and why it outputs 8 for up and down?
image

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cchaos
Copy link
Contributor

cchaos commented Feb 21, 2019

Looking at your last comment, you can see that the white line indicating your hover point is lined up with the peaks of both the green and red series. There are a couple reasons for confusion but I'm not exactly sure what yours is.

  1. The curve types of the red and green series are different and so even though they have the same values, they don't line up.
  2. How could it be both up and down at the same time? Depends on what the 8 (count) signifies.... And by glancing at the graph, I have no clue 😉

@snide
Copy link
Contributor Author

snide commented Feb 21, 2019

Heh. Yeah, I was mostly questioning the data in the graph to your second point

How could it be both up and down at the same time? Depends on what the 8 (count) signifies.... And by glancing at the graph, I have no clue

This seems like a bug or is a really confusing way to present the data?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

it('returns expected result for no status filter', async () => {
expect.assertions(2);
const search = jest.fn();
search.mockReturnValue({
Copy link
Contributor

Choose a reason for hiding this comment

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

snapshot rather than mock? might make maintenance easier

Copy link
Contributor

@mattapperson mattapperson left a comment

Choose a reason for hiding this comment

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

Approved but would like to see component names not have Query in the name, and query object not having a prefix of "get" making it look like a method/getter

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@justinkambic justinkambic merged commit f9bdf89 into elastic:master Apr 4, 2019
@justinkambic
Copy link
Contributor

Backported to:
7.x/7.1.0 840b788
#34574

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:enhancement Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.2.0 v8.0.0
Projects
None yet
6 participants