-
-
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
Unified hover: use legend.tracegroupgap
in mock legend
#6875
base: master
Are you sure you want to change the base?
Conversation
Thanks for the PR. |
@@ -1139,7 +1139,7 @@ function createHoverText(hoverData, opts) { | |||
bgcolor: hoverlabel.bgcolor, | |||
bordercolor: hoverlabel.bordercolor, | |||
borderwidth: 1, | |||
tracegroupgap: 7, | |||
tracegroupgap: fullLayout.legend ? fullLayout.legend.tracegroupgap : undefined, |
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.
Hmm I see we have the same logic on the next line for traceorder
but this most likely predates multiple legends. I suppose this is going to give the correct behavior 99% of the time, but at some point we may want to look for which legend say the first trace shown in hover is included in, and use the tracegroupgap
from that one.
Do we want to fall back on undefined
or would the previous default of 7
be better if there's no legend at all? Or maybe change it to 10
, which is the default tracegroupgap
when we DO have a legend, so adding an unstyled legend won't change the behavior here?
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.
@vladsavelyev any thoughts about @alexcjohnson 's comment? thanks - @gvwilson
When using a mock legend in the
unified
hover label, read the user-providedlegend.tracegroupgap
parameter instead of hardcodingtracegroupgap: 7
. To make the tooltip match the legend.