Fix hover label positioning for bar trace with set width
#1527
Conversation
- so that traces with set 'width' (array or scalar) use the correct bar span.
src/traces/bar/hover.js
Outdated
| var dx, dy; | ||
| if(trace.orientation === 'h') { | ||
| dx = function(di) { | ||
| // add a gradient so hovering near the end of a | ||
| // bar makes it a little closer match | ||
| return Fx.inbox(di.b - xval, di.x - xval) + (di.x - xval) / (di.x - di.b); | ||
| }; | ||
| dy = function(di) { | ||
| dy = function(di, i) { |
There was a problem hiding this comment.
I suppose this (providing i as an extra arg) is the most flexible way to handle the issue for future extensibility, but the way that matches better with what we do in scatter - and should be slightly better perf - would be to include the width of each point in di
There was a problem hiding this comment.
Yeah. Good call! Post #1519 this will be easy!
| @@ -392,9 +392,11 @@ function setBarCenter(gd, pa, sieve) { | |||
| for(var j = 0; j < calcTrace.length; j++) { | |||
| var calcBar = calcTrace[j]; | |||
|
|
|||
| // store the actual bar width and position, for use by hover | |||
| var width = calcBar.w = (barwidthIsArray) ? barwidth[j] : barwidth; | |||
There was a problem hiding this comment.
I suspect we could clean up the hover routine quite a bit more if we stopped using .x and .y in calcdata... then we'd only have to switch at the beginning of the routine "are xval and yval position and size or vice versa" but I wasn't sure what the consequences of that would be beyond hover, just adding one more value was safe and easy.
| * Nearly always it's the group that matters, but in case the bar | ||
| * was explicitly set wider than its group we'd better accept the | ||
| * whole bar. | ||
| */ |
There was a problem hiding this comment.
fixes the "compare" part of #1317 - at least if the bar is wider than our default. I guess if it's narrower and there are other things on the plot, we could be accepting this point when we shouldn't be... maybe I'll look at that case tomorrow and see if I can trigger it.
There was a problem hiding this comment.
Alright, the only case I can see where this pops up is an explicitly narrow (width<1) single bar with other things on the plot as well, like:
var data = [
{type: 'bar', x: [1], y: [2], width: 0.1}, {x:[1.1], y:[1]}
];
var layout = {
xaxis: {
range: [-2, 2]
}
};
Plotly.newPlot(gd, data, layout);then the hoverable region for the bar is so wide it actually extends past the scatter point. But the scatter point still wins when you'd want it to. So if that's all it is, I don't feel like it's worth addressing this very obscure edge case, it'd be just as likely to cause other problems.
There was a problem hiding this comment.
I agree with ⬆️ . Thanks for documenting it!
| @@ -61,7 +59,7 @@ module.exports = function plot(gd, plotinfo, cdbar) { | |||
| // log values go off-screen by plotwidth | |||
| // so you see them continue if you drag the plot | |||
| var p0 = di.p + ((poffsetIsArray) ? poffset[i] : poffset), | |||
| p1 = p0 + ((barwidthIsArray) ? barwidth[i] : barwidth), | |||
| p1 = p0 + di.w, | |||
There was a problem hiding this comment.
a little follow-on benefit of adding di.w
|
@etpinard back to you for review. Do the tests I added satisfy the TODO up top? |
Absolutely. 💃 thanks for wrapping this one up 🎉 |
fixes #1317 - at least the
hovermode: 'closest'part, see #1317 (comment) for more details.TODO: