sourcegraph / sourcegraph-public-snapshot

Code AI platform with Code Search & Cody
https://sourcegraph.com
Other
10.11k stars 1.29k forks source link

Make the graph x-axis labels match points, not the first of the month #21833

Open Joelkw opened 3 years ago

Joelkw commented 3 years ago

Make the graph x-axis labels match points, not the first of the month (this can wait until after beta, and after we're populating backend points). So here the label should be April 26, not just May 01 image

github-actions[bot] commented 3 years ago

Heads up @joelkw @felixfbecker - the "team/code-insights" label was applied to this issue.

unclejustin commented 3 years ago

After chasing a red herring and reading docs for the better part of the day I've concluded that this is working correctly. We can possibly tweak it, but I'm not sure that would be better.

What do you mean working correctly?!?

Let's look at some examples from Storybook

What's going on?

Here is how things work currently.

original

Wait this will be easier to see with columns

columns

So what's going on here? All of the data points in my sample set are about 7:21 PM. Or about 80% through the day. Which lines up well with the grid. Here I'll set all of them to midnight to drive the point home.

midnight

Oooooo pretty!

So we can see that the data points and the labels are actually in the correct place.

Then why is it so confusing?

Well we don't have all of the data displayed. The ticks on the x axis are truncated for legibility. D3 is intelligently removing some of the ticks to make room. So what you end up with is data points that don't properly line up with every tick. This can be pretty confusing, especially if we don't surface all of the data in the hover.

Where do we go from here?

We don't have a lot of options, but here are some off the top of my head:

Add the column lines.

I think this makes it a bit more clear what's going on, but doesn't really "fix" the problem.

Add more detailed info in the tooltip

This might make it more obvious as well. In my examples, we only ever show the day, not the time. So looking at "May 6, 7:21 PM" being closer to May 7 appears wrong. But if we show the time in the tooltip I suspect our users may have that "aha" moment where it all makes sense.

Normalize the data.

Basically round to the nearest interval. Not super straightforward since D3 is doing this for us with the power of magic, but I think we can extract the interval. Once we have it, we actually fudge the data to be rounded. Midnight for day intervals, hours for minute intervals, 1st of the month for months, etc.

I don't like this idea because we will be changing the actual data that is returned. I think our customers would get upset if the data being displayed isn't the "real" data. For example, if we normalize down then 1/1/2020 11:59 PM becomes 1/1/2020 12:00 AM which is a VERY different data point. If we round to closest then it is very possible/likely to have multiple data points stack on top of each other and get "lost".

Show all of the ticks.

Right now we limit the number of ticks displayed based on the width of a tick and the width of the container. This is to avoid text running into each other like this:

gross

This one is pretty much a non-starter.

Hand this over to @vovakulikov

This is probably the best option. Vova has a lot more experience with Visx than I do and may have a better solution at the ready.

What do you think @Joelkw?

cc @AlicjaSuska for design consideration.

vovakulikov commented 3 years ago

Nice comment @unclejustin. I like the idea with vertical x-axes lines and don't like the idea of changing the dataset to fit the point in the right position on the chart. I can remember that we had a chat with @AlicjaSuska about vertical lines but I can't remember what kind of concerns we had. @AlicjaSuska wdyt about x-axes ticks?

Joelkw commented 3 years ago

@unclejustin thanks for this great writeup, sorry I'm just catching up on it now. Here's a new wrinkle that might make this a little less "bad", at least for the next few releases: here and down in this slack thread is basically where we align (for backend insights, at least) to show data points on the first of the month – and since it seems like the current system naturally tries to preserve the 1st of the month in its ticks (see very first screenshot in this issue) then I think that would help/resolve this naturally.

Given that, and the fact that this is explicitly not as important as things like loading state, whatever frontend comes out of this (if it ends up being in scope on the backend), and any other issues still open in the beta tracking issue, I'm fine if we want to explicitly delay further progress on this so you can focus on loading state and/or jumping on any other beta issues that might be good (I defer to @felixfbecker on this, since you're still onboarding and he may have ideas).

If/when we do get to this, I might add a proposal to your list:

Make it so the "preserved" x-axis ticks are the day dates matching the datapoints

(If this is possible) I think this would solve all but the "an insight running daily" case – the issue with the first screenshot is more "my datapoints are on the 26th but my ticks are 5 days later" and less/not "even if the ticks were the 26th it would look off"; at pretty much anything beyond a single day per point, your datapoint will always definitionally be closer to the correct day tick than any other tick, so you won't really be able to tell it's "slightly off" depending on the hour of the datapoint. (So this would mean the only labels I see on the first screenshot would all be "26 May" "26 Apr" etc.)

And then for the "insights running daily case" (your screenshot examples) I wonder if we should then take your proposal to "add more detailed info in the tooltip" and in that case expose the hour in the tooltip hover – my hypothesis is that if you're running/checking insights daily you might actually care about the hour it ran, so we should explicitly show you rather than make you guess "how far along the interval is it" visually to guesstimate an hour yourself. Thoughts?

Re your proposals, I agree we shouldn't normalize the data.

unclejustin commented 3 years ago

@Joelkw roger that, focusing on the loading indicator.

Make it so the "preserved" x-axis ticks are the day dates matching the datapoints

This should be possible. I know in v1 of D3 we had complete control over the axis.

⚠️ Caution: Brain dumb below this point

We could let D3/Visx generate the x-axis as per normal, but then go in and "hide" the ticks that we don't want. Could be as simple as just hide every other point. We'll need to play with some scenarios if we go this route.

The ideal solution though is that the backend is batching these changes up into meaningful intervals and we just let D3 do its thing.