Repository navigation
Conversation
hoverlabel
|
Thanks for the PR! Our team will review and follow up. |
|
BTW, I just realized that there are currently no controls for the width of either the main hover label or the secondary hover label, and there is also no automatic text wrapping. This has actually been requested before: Width / automatic wrapping
#3800 was closed during the repository cleanup, but the requested I think it may make sense to add: hoverlabel: {
width: 200,
maxwidth: 300
}where Hover label positioningWhile looking into this, I also noticed that the positioning problem I reported in #7330 is closely related to the much older issue #381:
#381 has been open since 2016 and includes a proposal for: with values such as:
ref: #381 (comment) IMHO, I think options: Above may also be worth considering but in separate follow-ups. |
|
Hey @Lexachoc thanks for the submission here! I was about to review it, but noticed there are conflicts with a previous PR. I'll jump on this for a closer look after the conflicts are cleared up. |
Oops, no problem, I'll let you know once I've resolved the conflicts. |
…custom-styles # Conflicts: # src/components/fx/hover.js # test/jasmine/bundle_tests/mathjax_test.js
|
@KoolADE85 I merged them. You can play with it using the mock below: {
"data": [
{
"x": [1, 2, 3, 4],
"y": [10, 15, 13, 17],
"mode": "markers",
"name": "$\\alpha_{1c} = 352 \\pm 11 \\text{ km s}^{-1}$",
"type": "scatter",
"marker": {
"opacity": 0.5
}
},
{
"x": [2, 3, 4, 5],
"y": [16, 5, 11, 9],
"mode": "lines",
"type": "scatter",
"opacity": 0.5
},
{
"x": [1, 2, 3, 4],
"y": [12, 9, 15, 12],
"mode": "lines+markers",
"type": "scatter",
"opacity": 0.707,
"marker": {
"opacity": 0.707
}
},
{
"x": [1, 2, 3],
"y": [1, 2, 3],
"opacity": 0.2,
"line": { "width": 10, "color": "red" },
"marker": { "size": 20, "color": "blue" }
}
],
"layout": {
"title": {
"text": "hovermode='closest'"
},
"hoverlabel": {
"borderwidth": 5,
"borderradius": 20,
"namecolor": "red",
"bgnamecolor": "yellow",
"shadow": "0 10px 4px rgba(0, 0, 0, 0.2)",
"borderpad": 15,
"namelength": -1
},
"hovermode": "closest"
}
}
However, I noticed two issues:
I reverted to the original hover.js from #8051 and reproduced the same issue:
LaTeX expressions are not rendered when using
I also reproduced this behavior using the original hover.js from #8051. Both are reproducible with #8051's original implementation, so they do not appear to be caused by this merge. |
KoolADE85
left a comment
There was a problem hiding this comment.
You picked up quite a big chunk of work here @Lexachoc - great job!
I tested it out pretty thoroughly. I think there are some details to think through so that the new attrs look good in a variety of configurations.
But overall, this will be a great feature!
| var nameBg = bgNameColor || Color.addOpacity(bgColor, 0.8); | ||
| if(nameColor === undefined) { | ||
| nameColor = | ||
| Color.wcagContrast(fallbackNameColor, nameBg) < 4.5 | ||
| ? Color.contrast(nameBg) | ||
| : fallbackNameColor; | ||
| } |
There was a problem hiding this comment.
I notice that this contrast fix will alter existing graphs when their trace colours do not meet the contrast standard.
For that reason, I would prefer to pull that change out of this PR and into its own issue where we can discuss some approaches. For those users who are happy with the way their graphs look, I'm cautious to casually change the colour-coding behaviour.
There was a problem hiding this comment.
Sure. I've opened #8149 for discussion. I'll remove the automatic contrast logic and test after addressing the other review comments.
| var borderPadDelta = unifiedBorderPad - HOVERTEXTPAD; | ||
| if(!unifiedBg.empty() && borderPadDelta) { | ||
| var bgX = Number(unifiedBg.attr('x')) || 0; | ||
| var bgY = Number(unifiedBg.attr('y')) || 0; | ||
| var bgWidth = Number(unifiedBg.attr('width')) || 0; | ||
| var bgHeight = Number(unifiedBg.attr('height')) || 0; | ||
|
|
||
| unifiedBg.attr({ | ||
| x: bgX - borderPadDelta, | ||
| y: bgY - borderPadDelta, | ||
| width: Math.max(0, bgWidth + 2 * borderPadDelta), | ||
| height: Math.max(0, bgHeight + 2 * borderPadDelta) | ||
| }); |
There was a problem hiding this comment.
In the "unified" hover modes, the positioning of the hover labels do not account for border padding. As a result, the popup will appear in an unexpected location.
Example with exaggerated padding (notice where the cursor is!):
I think the unit tests could check for position here too, to catch any regressions.
Also on this topic: rounded corners are quite pixelated in this mode! This seems to be because the box is drawn with shape-rendering: crispEdges. I think we should use geometricPrecision or auto instead.
| d.bx = tbbWidth + 2 * borderPad; | ||
| d.by = Math.max(tbbHeight + 2 * borderPad, tx2height); |
There was a problem hiding this comment.
This doesn't take into account the border width, so it's possible for a thick border to clip some of the label text:
Annotations include the border in their size: borderfull = borderwidth + borderpad (draw.js#L218).
This would be a good candidate for a unit test as well!
| var nameRect = g.select('rect').call( | ||
| Drawing.setRect, | ||
| pX(shiftX.text2ShiftX + ((shiftX.alignShift - 1) * d.tx2width) / 2 + offsetX), | ||
| pX(rectX), | ||
| pY(offsetY - d.by / 2 - 1), | ||
| pX(d.tx2width), | ||
| pX(rectWidth), | ||
| pY(d.by + 2) | ||
| ); |
There was a problem hiding this comment.
The name box doesn't include the border or the radius which makes it look misshapen.
Without a stroke, the outline makes the name box appear to be a different size than the main box.
Example: (hoverlabel: { borderwidth: 8, borderradius: 20, bgnamecolor: 'black' })

With borderradius, it gets a bit trickier. There is no border rounding where the main box joins with the name box. These corners are square and, when there isn't any bgnamecolor, it looks like the rounding only applied to half of the label
Example: hoverlabel: { borderwidth: 4, borderradius: 12, borderpad: 12 }

| borderWidth: hoverItem.borderWidth, | ||
| borderRadius: hoverItem.borderRadius, | ||
| borderPad: hoverItem.borderPad, | ||
| nameColor: hoverItem.nameColor, | ||
| bgNameColor: hoverItem.bgNameColor, | ||
| shadow: hoverItem.shadow, |
There was a problem hiding this comment.
These options are supported on every trace with hoverlabel but not in sankey. Can we also support sankey? #7451 had the same discussion for showarrow.
| borderwidth: extendFlat({}, hoverLabelAttrs.borderwidth, { | ||
| description: 'Sets the border width (in px) of the hover labels for this trace.' | ||
| }), | ||
| borderradius: extendFlat({}, hoverLabelAttrs.borderradius, { | ||
| description: 'Sets the border radius (in px) of the hover labels for this trace.' | ||
| }), | ||
| borderpad: extendFlat({}, hoverLabelAttrs.borderpad, { | ||
| description: 'Sets the padding (in px) between the text and the border of the hover labels for this trace.' | ||
| }), | ||
| namecolor: extendFlat({}, hoverLabelAttrs.namecolor, { | ||
| description: 'Sets the text color of the secondary hover label for this trace.' | ||
| }), | ||
| bgnamecolor: extendFlat({}, hoverLabelAttrs.bgnamecolor, { | ||
| description: 'Sets the background color of the secondary hover label for this trace.' | ||
| }), | ||
| shadow: extendFlat({}, hoverLabelAttrs.shadow, { | ||
| description: 'Sets the shadow of the hover labels for this trace.' | ||
| }), |
There was a problem hiding this comment.
Other attributes support arrayOk for per-point array values, but these ones are excluded. Is that intentional? What do you think about supporting arrays here?
| return path + 'Z'; | ||
| } | ||
|
|
||
| function roundedSideArrowPath(width, height, horzSign, offsetX, offsetY, radius, roundOppositeSide, pX, pY) { |
There was a problem hiding this comment.
The rounded paths add three helpers (roundedRectPath, roundedSideArrowPath and roundedTopBottomArrowPath). The places that use these functions still keep the original hand-written paths using an if/else to decide which one to use.
Could we reduce duplication by creating a single helper that generically rounds the corners of a list of points?
Example fn signature:
function roundedPolygonPath(
points: {x: number, y: number}[],
radius?: number | number[] = 0,
scaleX?: number = 1,
scaleY?: number = 1
): string;An array radius would be per vertex so the arrow corners can stay sharp.
A radius of 0 would be "square" (effectively a no-op for this fn).
There was a problem hiding this comment.
The new tests only set the new hover options for the whole chart (layout.hoverlabel). Could we add a test that sets one on a single trace and checks it overrides the chart-wide value?






Addresses some requests in #2342 without closing it.
This PR adds several new
hoverlabelstyling options based on the discussion in #2342.New options:
hoverlabel.borderwidth— sets the hover label border widthhoverlabel.borderradius— sets the hover label border radiushoverlabel.borderpad— sets the padding between the text and the borderhoverlabel.namecolor— sets the secondary hover label text colorhoverlabel.bgnamecolor— sets the secondary hover label background colorhoverlabel.shadow— sets a drop shadow on the hover labelAdditional improvement
If
hoverlabel.namecoloris not specified, the trace color is preserved when it has sufficient contrast against the secondary label background (WCAG 4.5:1). Otherwise, a contrasting text color is used automatically.(Not sure whether this contrast improvement change belongs in this PR. Happy to move it out if preferred.)
Before/After
See the comparison in #2342 (comment).