Skip to content

mgr/dashboard: Landing Page improvements#36476

Merged
LenzGr merged 1 commit intomasterfrom
42072-landing-page
Aug 17, 2020
Merged

mgr/dashboard: Landing Page improvements#36476
LenzGr merged 1 commit intomasterfrom
42072-landing-page

Conversation

@alfonsomthd
Copy link
Copy Markdown
Contributor

@alfonsomthd alfonsomthd commented Aug 5, 2020

Fixes: https://tracker.ceph.com/issues/42072
Signed-off-by: Alfonso Martínez almartin@redhat.com

NOW:
Screenshot from 2020-08-13 14-05-05

BEFORE:
landing-page-1-master

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@alfonsomthd alfonsomthd requested review from a team, epuertat and tspmelo August 5, 2020 15:10
@alfonsomthd alfonsomthd added the skip-teuthology For PRs whose changes do not have an effect on QA runs/changes are not being tested in Teuthology label Aug 5, 2020
@alfonsomthd alfonsomthd requested a review from LenzGr August 5, 2020 15:11
@alfonsomthd alfonsomthd force-pushed the 42072-landing-page branch 2 times, most recently from c13fb74 to 0737edc Compare August 6, 2020 09:12
Copy link
Copy Markdown
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

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

LGTM!

@tspmelo
Copy link
Copy Markdown
Contributor

tspmelo commented Aug 6, 2020

Is it possible to add a debounce or timeout for the chart hover?
would be great if the popover didn't show when you are just passing though the charts

@alfonsomthd alfonsomthd force-pushed the 42072-landing-page branch 2 times, most recently from 43d629b to 429e617 Compare August 11, 2020 06:47
@alfonsomthd
Copy link
Copy Markdown
Contributor Author

Is it possible to add a debounce or timeout for the chart hover?
would be great if the popover didn't show when you are just passing though the charts

@tspmelo Done: now the popover shows only when clicking on the doughnut slice.

@alfonsomthd
Copy link
Copy Markdown
Contributor Author

jenkins test make check

Copy link
Copy Markdown

@nizamial09-zz nizamial09-zz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@tspmelo
Copy link
Copy Markdown
Contributor

tspmelo commented Aug 12, 2020

@alfonsomthd I'm guessing the following error:
image
Only one should be displayed. In other charts, I'm only getting the black tooltip.

@alfonsomthd
Copy link
Copy Markdown
Contributor Author

@alfonsomthd I'm guessing the following error:
image
Only one should be displayed. In other charts, I'm only getting the black tooltip.

@tspmelo Done: as per our previous conversation, only the Logs tooltip will be shown in that card as it was a requested feature.

@tspmelo
Copy link
Copy Markdown
Contributor

tspmelo commented Aug 12, 2020

Just noticed that the Objects chart is the only one that doesn't show the value in the labels.
Should we fix this? or will the label be too long?

@LenzGr
Copy link
Copy Markdown
Contributor

LenzGr commented Aug 12, 2020

Tested this locally today, nice work @alfonsomthd! This is definitely an improvement. Somehow tooltips were not displayed in my test environment, not sure why. But that explains why the mouse cursor changed from an arrow to a pointing finger when I hovered above the chart widgets.

Some minor observations:
The health state has a frame drawn around it when the cluster is in HEALTH_WARN state. Is this intentional to indicate that the status display is clickable?
Screenshot from 2020-08-12 16-51-29

In Chromium, the "PGs per OSD" label overflows the widget (it's fine in Firefox):
Screenshot from 2020-08-12 16-52-57

I think we discussed this during the standup already, but I would find it cleaner if there is no horizontal line separator between the label and the content. In Chrome, these lines also overflow the content box on the right:

Screenshot from 2020-08-12 17-03-24

@alfonsomthd
Copy link
Copy Markdown
Contributor Author

alfonsomthd commented Aug 13, 2020

The health state has a frame drawn around it when the cluster is in HEALTH_WARN state. Is this intentional to indicate that the status display is clickable?

I didn't touch it: this has been there for a long time, intentional.

In Chromium, the "PGs per OSD" label overflows the widget (it's fine in Firefox):

@LenzGr Can you check the zoom? And the screen resolution you use?

I think we discussed this during the standup already, but I would find it cleaner if there is no horizontal line separator between the label and the content. In Chrome, these lines also overflow the content box on the right:

I remember you pointing this out, but wasn't sure about the quorum: I'll remove it!

@alfonsomthd
Copy link
Copy Markdown
Contributor Author

Just noticed that the Objects chart is the only one that doesn't show the value in the labels.
Should we fix this? or will the label be too long?

@tspmelo Done: added values to objects legend.

@LenzGr
Copy link
Copy Markdown
Contributor

LenzGr commented Aug 13, 2020

The health state has a frame drawn around it when the cluster is in HEALTH_WARN state. Is this intentional to indicate that the status display is clickable?

I didn't touch it: this has been there for a long time, intentional.

OK, let's leave it as is then. It may make help users to realize that it's actually clickable.

In Chromium, the "PGs per OSD" label overflows the widget (it's fine in Firefox):

@LenzGr Can you check the zoom? And the screen resolution you use?

So it was not a zoom issue, both Firefox and Chromium rendered the page at the 100% setting. However, I do have the GNOME accessibility setting "Large Text" enabled, which AFAIK changes the DPI settings and this causes the weird rendering on Chrome (right window):

Screenshot from 2020-08-13 12-03-45

Without "Large Text", it looks fine:

Screenshot from 2020-08-13 12-02-09

I think we discussed this during the standup already, but I would find it cleaner if there is no horizontal line separator between the label and the content. In Chrome, these lines also overflow the content box on the right:

I remember you pointing this out, but wasn't sure about the quorum: I'll remove it!

Thanks! I think it looks better without them.

Copy link
Copy Markdown
Contributor

@LenzGr LenzGr left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Contributor

@tspmelo tspmelo left a comment

Choose a reason for hiding this comment

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

these 3 lines doesn't seem to do anything.

Fixes: https://tracker.ceph.com/issues/42072
Signed-off-by: Alfonso Martínez <almartin@redhat.com>
@alfonsomthd
Copy link
Copy Markdown
Contributor Author

jenkins test api

@alfonsomthd
Copy link
Copy Markdown
Contributor Author

jenkins test make check

@alfonsomthd
Copy link
Copy Markdown
Contributor Author

jenkins test dashboard

@LenzGr LenzGr merged commit 369252b into master Aug 17, 2020
@LenzGr LenzGr deleted the 42072-landing-page branch August 17, 2020 08:43
@LenzGr
Copy link
Copy Markdown
Contributor

LenzGr commented Aug 17, 2020

This PR also addresses one suggestion about improving the raw capacity widget from this issue: mgr/dashboard: Enhance info shown in Landing Page cards 'PGs per OSD' & 'Raw Capacity'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dashboard needs-review skip-teuthology For PRs whose changes do not have an effect on QA runs/changes are not being tested in Teuthology

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants