-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Output bounding box to hover event data #5512
Conversation
One thing I've noticed about this PR is that Dash rejects any nested properties so that adding |
FYI that's here - if a flat object is too cumbersome to work with we can certainly carve out an exception in |
Hey guys, what's the status of this PR? |
This is to feed plotly/dash-core-components#940 right? |
@archmoj @alexcjohnson what's left here? Can we merge this for 2.3.0? |
src/components/fx/helpers.js
Outdated
if ('x0' in pt) out.x0 = pt.x0; | ||
if ('x1' in pt) out.x1 = pt.x1; | ||
if ('y0' in pt) out.y0 = pt.y0; | ||
if ('y1' in pt) out.y1 = pt.y1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌴 there's 7 or 8 of these... could be something like copyCoords(pt, out)
- could even include the xa/ya
-> xaxis/yaxis
part, AFAICT only scattercarpet
is missing that one, but I bet it wouldn't even hurt to include that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that information already accessible through plotly_hover
? If so, can't we just modify dcc.Graph
instead of making this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original mouse event is accessible in plotly_hover
, and that contains the pointer location in various coordinate systems. But plotly.js hover events are more sophisticated, and it would be far better if we can match the plotly.js behavior with our new interface.
- These coordinates mark the data points rather than the mouse.
- We give the full extent of the location being hovered on. For example, for a vertical bar this is the left and right edges of the top of the bar. For a scatter trace, this is the bounding box of the marker.
- There can be multiple points in the hover set, each with their own bounding box.
src/traces/pie/event_data.js
Outdated
v: pt.v, | ||
|
||
// TODO: These coordinates aren't quite correct and don't take into account some offset | ||
// I still haven't quite located (similar to xa._offset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this doesn't work correctly yet, can we just drop it and leave pie as a TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 03b22ec.
test/jasmine/tests/ternary_test.js
Outdated
var SORTED_EVENT_KEYS = [ | ||
'data', 'fullData', 'curveNumber', 'pointNumber', 'pointIndex', | ||
'xaxis', 'yaxis', 'a', 'b', 'c', | ||
'bbox', 'x0', 'x1', 'y0', 'y1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need x0
, x1
, y0
and y1
keys here?
They are available in bbox
.
src/traces/bar/event_data.js
Outdated
if('x1' in pt) out.x1 = pt.x1; | ||
if('y0' in pt) out.y0 = pt.y0; | ||
if('y1' in pt) out.y1 = pt.y1; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need x0
, x1
, y0
and y1
in event data? Having bbox
in not enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we concluded that more was better here, to make it easier for folks in Dash-land.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? For vertical bars y0
is equal to y1
here and in the bbox
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't p0
, p1
, s0
and s1
used instead of (x|y) (0|1)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For vertical bars
y0
is equal toy1
Yes, y0===y1
for vertical bars. We want the label to be at the end of the bar, not in the middle.
Shouldn't
p0
,p1
,s0
ands1
used instead
The final event data should contain x/y. p/s is not useful to the user for positioning other elements, and that's the purpose here.
The x0, y0, x1, y1 positions outside |
Yes, that's OK - currently dcc.Graph drops nested objects from the event so it doesn't try to serialize xaxis & yaxis objects, but in the tooltip PR (which I'm going to have to remake in the monorepo anyway) we're whitelisting this object.
That's fine for right now, but please make an issue to come back to this later and add support for other traces. I'm particularly expecting people will want this to work in pies and 3D. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💃
The original issue for this was for 3D... this PR doesn't work for 3D scatter? |
Right now marker sizes are take into account for |
For |
y0===y1 isn't a big deal - mostly people will be using the average y value and depending on x0 & x1 to specify the horizontal extent. I wouldn't spend much time investigating all of these now, but later, if folks on the Dash side want to be able to position hover labels above or below the point instead of left/right, we can revisit this as a minor bug. |
|
Addressing #1323
This PR adds
bbox: {x0, x1, y0, y1}
data to the points of hover event data. I've so far opted to keep the same format as the original hover data since I think it's very easy understand and doesn't mix coordinates with information about the righthand side of the plot, which I think is best left to the user. (Or at the very least would be useful additional/duplicate information while leaving x0/x1/y0/y1 available.)A short todo list:
A list of related tasks:
Regarding the last point, the coordinates are basically
gd.offsetTop + gd.clientTop + offsetWithinPlot
. The benefit of this is that positioning hovers within the plot should be very simple. You simply place a hover element as a sibling of the graph div, assign it the coordinates, and it should work without needing to worry about the margins, borders, or the offset parent (nearest element with position: 'relative' | 'absolute'). The downside is that this brings in position information from outside the graph div itself, but I think a reasonable solution is to state that if you want coordinates strictly limited to the plot, you should place HTML hover content and the graph div as direct children of a relative|absolute-positioned element.Regarding CSS transforms, I think that I just need to apply
layout._invTransform
to the coordinates within the plot, but I haven't yet confirmed this. In matrix pseudocode, that would make the coordinates[offsetLeft, offsetTop] + [clientLeft, clientTop] + invTransform * [plotOffsetLeft, plotOffsetTop]
.The current list of trace types supported is:
contourcarpet(no hover info)indicator(does not apply?)parcoords(does not apply?)table(does not apply)Obviously some of these aren't so important, but I think if there are some not worth supporting (e.g. pointcloud?), it'd at least be nicely to clearly state expectations around what does happen. I think it should not be too bad to fix the GL plots, which probably mostly goes through loneHover.
Some of them (e.g. ohlc) also require particular knowledge of what the hover points mean since you get a flat list of maybe five points (bounding box, o, h, l, c?), but I think the main thing for now is that the data is there.