-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: Make hover label triangle optional #7451
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
base: master
Are you sure you want to change the base?
Conversation
@emilykl |
@archmoj not quite ready for review but I'll merge |
1126715
to
63e44ab
Compare
'V' + pY(offsetY - HOVERARROWSIZE) + | ||
'Z')); | ||
pathStr = 'M0,0L' + pX(horzSign * HOVERARROWSIZE + offsetX) + ',' + pY(HOVERARROWSIZE + offsetY) + | ||
'v' + pY(d.by / 2 - HOVERARROWSIZE) + |
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.
OMG math
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.
@gvwilson Coming up with the new SVG path string with no arrow was Claude Code's only helpful contribution to this PR (but it's a doozy, so, worth it)
@archmoj This is ready for review. A couple questions for you —
|
One could possibly add arrayOk to the Line 89 in 2cac1b1
For this PR I don't see a strong use case for that option. So IMHO it's a good idea to set arrayOk to false as you did.
|
We could handle this option for parcats as follow: hoverItems.trace._hoverlabel = {showarrow: false}; before calling Then make the following change var showArrow = (d.trace._hoverlabel || d.trace.hoverlabel)?.showarrow; |
In addition to |
Oh that's clever, I like that. Maybe for another PR. It would be cool too if we could just support all the |
Instead of the solution proposed above, exports.loneHover = function loneHover(hoverItems, opts) {
...
var gd = opts.gd; |
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.
This looks good to me. I left some suggested feedback for you to consider.
pathStr = 'M-' + pX(d.bx / 2 + d.tx2width / 2) + ',' + pY(offsetY - d.by / 2) + | ||
'h' + pX(d.bx) + 'v' + pY(d.by) + 'h-' + pX(d.bx) + 'Z'; |
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.
Would you be up for using a template literal here and elsewhere?
pathStr = 'M-' + pX(d.bx / 2 + d.tx2width / 2) + ',' + pY(offsetY - d.by / 2) + | |
'h' + pX(d.bx) + 'v' + pY(d.by) + 'h-' + pX(d.bx) + 'Z'; | |
pathStr = `M-${pX(d.bx / 2 + d.tx2width / 2)},${pY(offsetY - d.by / 2)}h${pX(d.bx)}v${pY(d.by)}h-${pX(d.bx)}Z`; |
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.
We could leave refactoring for another PR.
Template literal may run slower: https://jsperf.app/es6-string-literals-vs-string-concatenation/43
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 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.
Have you seen any issues with this kind of calculation? I imagine that most computers are powerful enough where this isn't an issue.
Co-authored-by: Cameron DeCoster <[email protected]>
Closes #7278
Adds
trace.hoverlabel.showarrow
andlayout.hoverlabel.showarrow
attributes (default:true
) to enable hiding the triangular carat on the hover text box.Also adds a Jasmine test for the new attributes.
With

showarrow: true
(default; same as current behavior):With

showarrow: false
: