Skip to content

fix ternary hover after zoom/pan events #688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 27, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/traces/scatter/hover.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,11 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
}

if(inside) {
// constrain ymin/max to the visible plot, so the label goes
// at the middle of the piece you can see
ymin = Math.max(ymin, 0);
ymax = Math.min(ymax, ya._length);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if you have, for example, a shape that goes way off the top of the plot, the label will still be centered on the visible portion rather than pinned to the top (and because of how this happens, the x position of the label could also have ended up screwy previously)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. That sounds good.

Now that all the scatter ternary hover label testing infrastructure is in place, would you mind adding a test case for this?


// find the overall left-most and right-most points of the
// polygon(s) we're inside at their combined vertical midpoint.
// This is where we will draw the hover label.
Expand All @@ -128,6 +133,10 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
}
}

// constrain xmin/max to the visible plot now too
xmin = Math.max(xmin, 0);
xmax = Math.min(xmax, xa._length);

// get only fill or line color for the hover color
var color = Color.defaultLine;
if(Color.opacity(trace.fillcolor)) color = trace.fillcolor;
Expand Down
25 changes: 21 additions & 4 deletions src/traces/scatterternary/hover.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,29 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
var scatterPointData = scatterHover(pointData, xval, yval, hovermode);
if(!scatterPointData || scatterPointData[0].index === false) return;

var newPointData = scatterPointData[0];

// if hovering on a fill, we don't show any point data so the label is
// unchanged from what scatter gives us.
if(scatterPointData[0].index === undefined) return scatterPointData;
// unchanged from what scatter gives us - except that it needs to
// be constrained to the trianglular plot area, not just the rectangular
// area defined by the synthetic x and y axes
// TODO: in some cases the vertical middle of the shape is not within
// the triangular viewport at all, so the label can become disconnected
// from the shape entirely. But calculating what portion of the shape
// is actually visible, as constrained by the diagonal axis lines, is not
// so easy and anyway we lost the information we would have needed to do
// this inside scatterHover.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO is fairly uncommon, but when it happens you can see things like this:
screen shot 2016-06-25 at 1 01 15 am
where the label is not touching the shape at all. Because it's rare, doesn't actually stop the user from getting the information, and the fix would be really hairy, I propose to ignore it 🙈

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with ignoring this. 👍

if(newPointData.index === undefined) {
var yFracUp = 1 - (newPointData.y0 / pointData.ya._length),
xLen = pointData.xa._length,
xMin = xLen * yFracUp / 2,
xMax = xLen - xMin;
newPointData.x0 = Math.max(Math.min(newPointData.x0, xMax), xMin);
newPointData.x1 = Math.max(Math.min(newPointData.x1, xMax), xMin);
return scatterPointData;
}

var newPointData = scatterPointData[0],
cdi = newPointData.cd[newPointData.index];
var cdi = newPointData.cd[newPointData.index];

newPointData.a = cdi.a;
newPointData.b = cdi.b;
Expand Down