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

New tooltips UI #4156 #4651

Merged
merged 20 commits into from
Jun 14, 2021
Merged

New tooltips UI #4156 #4651

merged 20 commits into from
Jun 14, 2021

Conversation

paolodamico
Copy link
Contributor

@paolodamico paolodamico commented Jun 8, 2021

Examples

Simple breakdown

Multiple series & math aggregation

Breakdown & math aggregation

Multiple series & breakdown

Lifecycle

Changes

Implements the new tooltips as designed on #4156 for trend graphs. The new design is not only more sleek and in line with the most recent UI changes, but it also introduces simplicity when reading graphs:

  1. We now clearly show breakdown values where applicable.
  2. Graph series are now smartly distinguished (e.g. breakdown vs having multiple graph series).
  3. Sets the ground for the improved legends table to be introduced later.
  4. Now supports showing a ton of graph series (height), and large widths in a nice way. For large heights, we display up to certain amount of elements (depending on the screen's height), and we sort to show first the most relevant items (i.e. ones that have larger values).

Large width

Large height

  1. Closes Feature flag 4156-improved-graphs bug reports #4647
  2. Shows vertical axis in a nice way when there are large numbers.

Before

After

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests
  • Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious
  • Frontend/CSS is usable at 320px (iPhone SE) and decent at 360px (most phones)

@paolodamico paolodamico added the highlight ⭐ Release highlight label Jun 8, 2021
@timgl timgl temporarily deployed to posthog-pr-4651 June 8, 2021 21:47 Inactive
@timgl timgl temporarily deployed to posthog-pr-4651 June 8, 2021 22:42 Inactive
@paolodamico
Copy link
Contributor Author

One thing I'm on the fence of, is the idea of having a horizontal crosshair / scrubber line and a single tooltip for each data point on the x-axis. If you have a ton of lines/series, the behavior feels strange, because for instance you may want to look at what's going on at a specific point, and this makes it very hard (or even impossible).

So I see 3 options:

  1. Keep it the way it is in this PR.
  2. Keep it the way it is currently in master (non-feature-flagged), hovering over each data point.
  3. H1@p which does something similar actually hides overflowing items in an "Other" category, we could consider something like this, but it would probably have to be a future improvement.

Thoughts? Particularly @corywatilo @mariusandra @samwinslow

@mariusandra
Copy link
Collaborator

Bug description

The perfect combination is when the list is very long and also quite wide:

2021-06-08 22 21 03

  • Problems with wrapping when viewing the rightmost values
  • Can't sometimes see the end of the list
  • The tooltip covers the entire graph

Limiting the breakdown count might be a option, but who knows what other large combination of data flows by.

Expected behavior

If I want my lines broken down by URL, I should get my lines broken down by URL, damnit! 😆

@paolodamico
Copy link
Contributor Author

Still not fully ready but that problem has already been addressed here 😉 @mariusandra

@timgl timgl temporarily deployed to posthog-pr-4651 June 9, 2021 21:50 Inactive
@timgl timgl temporarily deployed to posthog-pr-4651 June 9, 2021 22:39 Inactive
@timgl timgl temporarily deployed to posthog-pr-4651 June 9, 2021 22:54 Inactive
@timgl timgl temporarily deployed to posthog-pr-4651 June 10, 2021 11:43 Inactive
@paolodamico paolodamico marked this pull request as ready for review June 10, 2021 11:43
@mariusandra
Copy link
Collaborator

Love the new design! Found some UX issues though. Haven't looked at the code yet :).

Am I alone in seeing something like this, where I only see the selected points in the tooltip?
2021-06-10 15 43 47

The bar charts get needlessly black and won't get selected if the mouse is further to the right than the center of the last bar. Same happens with the first bar.
2021-06-10 15 45 03

It's a bit weird with the "bar value" graphs
2021-06-10 15 46 22

Hard to know what is what with "compare previous"
image

The tooltip can look slightly messy and unaligned with long values. Could we somehow "table" them?
image

If I click now, what users will I see? For which breakdown? Random? All? First? Perhaps it makes sense to only open users if you click a specific label on the tooltip?
image

At 320px it's not usable with a breakdown:
image

@mariusandra
Copy link
Collaborator

mariusandra commented Jun 10, 2021

Also, what is "day 4"? I need to count on 10 fingers back from today to get the actual date? :)
image

This is pretty hard to read:
image

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Found a bunch of things that seem not right. Otherwise looking good :)

@timgl timgl temporarily deployed to posthog-pr-4651 June 10, 2021 19:01 Inactive
@timgl timgl temporarily deployed to posthog-pr-4651 June 10, 2021 19:53 Inactive
@paolodamico
Copy link
Contributor Author

Thanks for the initial review @mariusandra. Comments below and ready for another look,

  1. Selected points in tooltips, hover in bar charts & bar value charts weirdness addressed.
  2. Compare previous issue is outside the scope of this PR. I actually noticed the problem while working through this, but it's something we need to address in a more general sense. As you can see the axis and legend table also contains this confusing nomenclature.
  3. Addressed properly supporting extra long values because there's always a Marius who wants to breakdown by current URL? 😂

4. Re unusuable at 320px, hmmm.... I would argue that devices that size don't have a cursor and therefore have no hover behavior? But feel free to pushback 5. The only outstanding point which I'm not entirely sure is the behavior when clicking a point if the graph has multiple series, perhaps we should show some selection option in the modal that opens up. Not sure if clicking on the specific point of the tooltip is best, what if the point is not shown? Wdyt @corywatilo ? Curious to hear your thoughts too if you have a few minutes to spare @samwinslow

@paolodamico paolodamico requested a review from mariusandra June 10, 2021 20:00
This was referenced Jun 10, 2021
@mariusandra
Copy link
Collaborator

Awesome! Things seem much better and the experience is already so much nicer than what we have now. Ok for the "compare previous" and other points not mentioned below:

  1. Breakdown value graph tooltips are funny in positioning.
    2021-06-10 22 55 59

  2. (Re: 5.) As for the clicking behaviour, it is a bit weird now because you don't even see what you're clicking on:

2021-06-10 23 01 04

For resolving it, can we do something similar to point 1 in this internal post where the tooltip is just a bit away from the line? Plus some CSS animations for extra smoothness :).

@mariusandra
Copy link
Collaborator

  1. No matter how we put it, if we click on lines, we're going to run into a case where you have two different lines (pageviews and whatnot) converging on the number "5" for the 3rd of May, and you want to see the users behind both of them. How to solve that? Two things come to mind: somehow clicking the line in the tooltip, or then giving the user another tooltip asking which "5" does she want to open.

  2. If I have a lot of urls broken down, I would also like to scroll inside the tooltip and not just see the first dozen.

Both those points push towards a solution where you can interact more with the contents of the tooltip.

@corywatilo
Copy link
Contributor

This looks really good!

Personally I do prefer the unified scrubber with the line that runs all the way through. I feel like I don't need to move my mouse all over the graph as much to make sure I have a good sense of the data.

A minor UX nit: When moving the mouse horizontally, the Y-axis line moves smoothly, but the actual tooltips are locked to the plots (eg day). It would be nice for either the tooltips to move smoothly, or for the Y-axis line to snap to each spot the tooltip will actually change.

image

@paolodamico
Copy link
Contributor Author

Thanks @mariusandra & @corywatilo for this round of reviews, great points! As these are UX and non-code blocking, I'll fix merge conflicts, go ahead and merge and address all your comments in a follow-up PR. Reason is this is already quite large, running into conflicts already, and there are 3 other PRs in this series that could cause further headaches.

For your peace of mind, this feature isn't released to anyone just yet.

@timgl timgl temporarily deployed to posthog-pr-4651 June 14, 2021 10:57 Inactive
@paolodamico
Copy link
Contributor Author

Failing cypress component tests are unrelated (already failing here https://github.com/PostHog/posthog/runs/2819066766). Seems first failure started in #4686.

@paolodamico paolodamico merged commit 0a1eb34 into master Jun 14, 2021
@paolodamico paolodamico deleted the tooltips-new-ui branch June 14, 2021 12:03
@paolodamico paolodamico mentioned this pull request Jun 14, 2021
6 tasks
@mariusandra
Copy link
Collaborator

If you'll fix bugs in this PR in new ones, I'll add one more weirdness to this issue:

  • Tooltips don't work on colored dashboard:

2021-06-14 14 16 26

This is the top chart, but it's the same with any other one.

@paolodamico paolodamico mentioned this pull request Jun 16, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
highlight ⭐ Release highlight
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature flag 4156-improved-graphs bug reports
4 participants