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

Trend charts look very broken with new dashboard grid changes #31628

Closed
cdeweyx opened this issue Jun 15, 2023 · 3 comments · Fixed by #31722
Closed

Trend charts look very broken with new dashboard grid changes #31628

cdeweyx opened this issue Jun 15, 2023 · 3 comments · Fixed by #31722
Assignees
Labels
.Regression/master Regression that is only present on master, or bug in new upcoming feature Reporting/Dashboards .Reproduced Issues reproduced in test (usually Cypress)
Milestone

Comments

@cdeweyx
Copy link

cdeweyx commented Jun 15, 2023

Context

We updated the dashboard grid row height in #31019 and it's impacting Trend charts to make them look broken. See the first three cards in the top row of this dashboard.

Expected Behavior

  • Update font size and spacing to look like it did before
  • Make sure this is the case as small as 2x2 (the new min size). We should be able to lean on similar logic that exists for Scalars which truncates the field name with ... when needed.
@cdeweyx cdeweyx added .Regression/master Regression that is only present on master, or bug in new upcoming feature Reporting/Dashboards .Team/42 🌌 labels Jun 15, 2023
@deniskaber deniskaber self-assigned this Jun 16, 2023
@NevRA NevRA assigned kamilmielnik and unassigned deniskaber Jun 19, 2023
@kamilmielnik
Copy link
Contributor

Recordings of behavior using various viewport widths for reference

The question is: "Orders" table from "Sample Database" - "count" summarized by "created at: year".

Before - 18 columns
before.mp4
Now - 24 columns
now.mp4

@kamilmielnik
Copy link
Contributor

kamilmielnik commented Jun 19, 2023

For better context please see the recordings above.

Update font size (...) to look like it did before

The font size of card title is dynamically calculated based on:

  • available horizontal space for the dashboard (dashboard is wider when the sidebar is closed, etc.)
  • how many columns does a card occupy
  • the title itself (text)

Differences in the font size are expected.

Update (...) spacing to look like it did before

Spacing is also dynamically computed based on available space. What we can do is to add minimum spacing, because now it's 0 and it does not look so good.

Make sure this is the case as small as 2x2 (the new min size). We should be able to lean on similar logic that exists for Scalars which truncates the field name with ... when needed.

The trend case seems to be more complex. I think these changes are necessary to address this:

  1. Add ellipsis to the value - show entire value in tooltip if necessary
  2. Allow only 1 line of text for the card title (currently 2 lines are allowed) when there's not enough room
  3. Define minimum vertical spacing within the card (4px sounds reasonable). And/or we could increase the line-height.
  4. Decrease horizontal spacing within the card.
  5. Hide the "was {value} last {period}" text when there's not enough room (but keep the % diff with the arrow if possible)
  6. Fix how "was {value} last {period}" breaks in multiple lines (remove the flexbox; see video of "now")
  7. Add ellipsis to the % diff or decrease the numerical precision (currently: 2 decimal spaces)

@cdeweyx Does the list look good to you ? Did I miss anything? cc: @NevRA

The most extreme case would look something like this:
image

@cdeweyx
Copy link
Author

cdeweyx commented Jun 19, 2023

Yeah this looks much better. Let's give this list a go. — Thanks for the write-up and investigation here.

@kamilmielnik kamilmielnik added this to the 0.47 milestone Jun 29, 2023
@kamilmielnik kamilmielnik added the .Reproduced Issues reproduced in test (usually Cypress) label Jul 5, 2023
This was referenced Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Regression/master Regression that is only present on master, or bug in new upcoming feature Reporting/Dashboards .Reproduced Issues reproduced in test (usually Cypress)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants