This change adds a UI element to the line graph that indicates the value being hovered, both in normal and breakdown view.
- 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
Build has FAILED
Link to build: https://integration.wikimedia.org/ci/job/phabricator-jessie-diffs/724/
See console output for more information: https://integration.wikimedia.org/ci/job/phabricator-jessie-diffs/724/console
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...!
I think we should stick to the convention within html tags, we should always use attribute="blah" without spaces around = and with " double quotes.
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?
Maybe graphModel.getActiveBreakdown().values.find(v => v.key === b).name could be inside a method like getColorForBreakdown().
I think we don't need to split here. I think this should work:
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...?
This line could be split or factored out, no? It's a bit too long...
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'd suggest that the circles have a higher z-index than the lines in the chart,
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.
- 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
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.
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.
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.
You're totally right! I didn't know this :)
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.
- 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.
- We should reserve the concept of "selectedPoint" for actually selecting a point, I would call this hoverPoint or something like that.
- 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
- 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.
- We need to revisit the colors, some of these are way too light to be seen on a white background.
LGTM! Thanks for all the changes :]
Thanks to take care of my nit-pickyness! :D
OK, sounds good. Let's do this in another task!
Is this to be done also in a separate task? If so, I'm OK!
Thanks! Definitely easier for me to (sort-of-)understand xD
Sure, let's create another task for that.
Build has FAILED
Link to build: https://integration.wikimedia.org/ci/job/phabricator-jessie-diffs/735/
See console output for more information: https://integration.wikimedia.org/ci/job/phabricator-jessie-diffs/735/console