Tooltip path #284
Tooltip path #284
Conversation
|
👍 I pulled down your branch on top of the latest here and it works great!
|
inst/js/tooltip.js
Outdated
| var tooltip; | ||
| tooltip_bg= tipG.append('path'). | ||
| attr("id", "tooltip-box"). | ||
| attr("class", "tooltip-box"); |
There was a problem hiding this comment.
good idea to get this on multiple lines.
the convention i've been seeing is to put the dot at the beginning of each new line (e.g. https://d3js.org/#introduction) - do you have an opinion on where the dots should go, and why?
also, it looks to me like there's nothing stopping you from continuing to declare and define each variable in a single command, which seems a little cleaner to me. Like this:
var tooltip_bg = tipG.append('path')
.attr("id", "tooltip-box")
.attr("class", "tooltip-box");
what do you think about doing it that way instead? are there reasons not to?
There was a problem hiding this comment.
I was following Mary's style for the var varname; style ones. But the PR is a mix of both. Not sure why to do it in two lines?
There was a problem hiding this comment.
I also prefer the .attr() line starts. I will clean that up.
inst/js/tooltip.js
Outdated
| halfLength = textLength / 2; | ||
| textBuffer = 6; | ||
| var tipYoffset = -2; // so that the tip is slightly above the mouse location | ||
| var tipTriangle = {x:6, y:10}; |
There was a problem hiding this comment.
good idea to extract & name all these constants
This is an add-on to #282 , but makes the tooltip box as a single instead of a (for the box) and a for the triangle tip.
This gets rid of the little bit of separation or overlap between the triangle and the box that appears in other vizzies:
