Page MenuHomePhabricator

Adds UI element on hover to line graph
ClosedPublic

Authored by fdans on Sep 22 2017, 9:55 AM.

Details

Reviewers
mforns
Milimetric
Commits
rWIKISTATSb123cb5be7ef: Adds UI element on hover to line graph
Patch without arc
git checkout -b D791 && curl -L https://phabricator.wikimedia.org/D791?download=true | git apply
Summary

This change adds a UI element to the line graph that indicates the value being hovered, both in normal and breakdown view.

Diff Detail

Event Timeline

fdans created this revision.Sep 22 2017, 9:55 AM
fdans updated this revision to Diff 2091.Sep 22 2017, 2:06 PM
  • removes popup ref
mforns requested changes to this revision.Sep 22 2017, 6:42 PM

Great code :] The d3 part looks tough...

Works as expected and I like the overall look.


I commented on what I think might be small improvements, but please let's discuss everything if you disagree!
Apart from what's in the comments, I've also seen that:

  • When hovering out of the chart quickly (mouseOut event does not trigger) and then going round it and activating/deactivating the breakdown, strange things are shown in the popup.

  • When granularity is monthly, with the breakdown on, the popup text is not completely aligned to the right. The difference is so small that I don't care, but just in case you know why this happens and can fix it in 1 second. Also curious that it does align it correctly with daily granularity...!

src/components/detail/chart/LineChart.vue
2–20

I think we should stick to the convention within html tags, we should always use attribute="blah" without spaces around = and with " double quotes.

4

I don't understand why selectedPoint.month? Is this used also with daily granularity?

EDIT: After having read all the code, I see that month was there before, because we only had monthly granularity. But now that we have also daily, it should be changed to something else no? Like date? datetime? timestamp?

10

Maybe graphModel.getActiveBreakdown().values.find(v => v.key === b[0]).name could be inside a method like getColorForBreakdown().
This way, the line wouldn't be so long :]

49

Semi-colon missing

65

I think we don't need to split here. I think this should work:
new Date(row.month)
no?

74

semi-colon missing

92

I think it would be helpful to add a couple comments to explain the d3 code a bit in this method. The d3 code is naturally of ugly-spaghetti kind, so maybe it could help...?

131

missing semi-colon

148

This line could be split or factored out, no? It's a bit too long...

235

When you hover along a chart, especially if the breakdown is active, there's a strange effect created by this 100ms animation, where for a moment you see lots of dots on the chart (those that have not faded out yet).
I would prefer not having animation here, but this is more of a personal opinion. We should ask Dan maybe :]

241

missing semi-colon

254

missing semi-colon

273

missing semi-colon

285

I'd suggest that the circles have a higher z-index than the lines in the chart,
but again, more of a personal pref.

298

Also, a suggestion, given that the box is fixed in position, we could add a bit of transparency to avoid hiding the lines in the chart.
Not sure though, because it might be that the colored lines interfere with the colored numbers...
But right now, especially if the breakdown is on, quite a big part of the chart can be hidden.

This revision now requires changes to proceed.Sep 22 2017, 6:42 PM
fdans updated this revision to Diff 2096.Sep 25 2017, 9:38 AM
fdans marked 10 inline comments as done.
  • improves html style
  • removes unnecessary string split
  • adds explanatory comments
  • removes animation from circles
  • puts circles on top of line
  • adds transparency to the box
  • removes selected point when chart is reloaded
  • fixes misalignment in tooltip
fdans added a comment.Sep 25 2017, 9:38 AM

Thank you so much @mforns for such a thorough review!! I've addressed or fixed all issues, both in inline comments and in your main comment. Let me know what you think about passing those couple changes to a different task.

src/components/detail/chart/LineChart.vue
4

Toootally agree, this is a property that we have since prototyping. I would prefer, however, to create a separate task to not only replace the names (as it would affect many files out of the scope of this revision), but also get rid of the YYYY-MM-DD timestamps that we're using currently in the pipeline between aqs and the graph.

10

yes, what I planned to do what to create a separate model class for Breakdown, since it would benefit from a couple methods like this.

65

You're totally right! I didn't know this :)

148

Yeah, this is the other example of why we need a separate breakdown class (breakdown.getColorFor(value)). Since this line was like this before this revision, I'd prefer, unless you feel strongly about it, to make that change in a different one.

Ok, this looks good and looks to cover Marcel's very thorough review. I have a couple of thoughts, but I wouldn't block a deployment because of them.

  1. In the design, the numbers show up on a div that moves to point to the part of the graph being hovered. I prefer that, and we can probably get it done with just some CSS hacks.
  2. We should reserve the concept of "selectedPoint" for actually selecting a point, I would call this hoverPoint or something like that.
  3. There are some optimizations for the hover detection in Limn that we should look at. We did it this way in Limn at first too, and it did the same thing it does here - spikes the CPU
  4. I don't love how we have to special case the breakdowns. That means the hover guide needs to know about the graph and the breakdown concept. It should be possible to make the hover guide a stand-alone component that can work with both the bar and the line chart. Which would be nice because the bar graph needs this guide also.
  5. We need to revisit the colors, some of these are way too light to be seen on a white background.
mforns accepted this revision.Sep 27 2017, 1:20 PM

LGTM! Thanks for all the changes :]

src/components/detail/chart/LineChart.vue
2–20

Thanks to take care of my nit-pickyness! :D

4

OK, sounds good. Let's do this in another task!

10

Is this to be done also in a separate task? If so, I'm OK!

92

Thanks! Definitely easier for me to (sort-of-)understand xD

148

Sure, let's create another task for that.

This revision is now accepted and ready to land.Sep 27 2017, 1:20 PM
fdans updated this revision to Diff 2108.Sep 27 2017, 4:42 PM
  • renames selectedpoint to hoveredpoint
  • makes lines more clear, adjusts color
fdans updated this revision to Diff 2110.Sep 28 2017, 3:23 PM
  • Merge branch 'develop' of ssh://git-ssh.wikimedia.org/source/wikistats into line-graph-ui
  • makes sure metric data is immutable
  • restores shouldbechecked
fdans updated this revision to Diff 2112.Sep 28 2017, 3:51 PM
  • fixes test
This revision was automatically updated to reflect the committed changes.