Skip to content

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

emilykl
Copy link
Contributor

@emilykl emilykl commented Jul 2, 2025

Closes #7278

Adds trace.hoverlabel.showarrow and layout.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):
Screenshot 2025-07-22 at 1 32 27 PM

With showarrow: false:
Screenshot 2025-07-22 at 1 32 36 PM

@emilykl emilykl marked this pull request as draft July 2, 2025 16:19
@gvwilson gvwilson added feature something new P1 needed for current cycle labels Jul 3, 2025
@archmoj
Copy link
Contributor

archmoj commented Jul 21, 2025

@emilykl
Is this PR ready for review?
Would you please merge master into this branch?
Thank you 🙏

@emilykl
Copy link
Contributor Author

emilykl commented Jul 22, 2025

@archmoj not quite ready for review but I'll merge master — will ping you later today when ready for review

@emilykl emilykl force-pushed the 7278-make-triangle-optional branch from 1126715 to 63e44ab Compare July 22, 2025 17:27
'V' + pY(offsetY - HOVERARROWSIZE) +
'Z'));
pathStr = 'M0,0L' + pX(horzSign * HOVERARROWSIZE + offsetX) + ',' + pY(HOVERARROWSIZE + offsetY) +
'v' + pY(d.by / 2 - HOVERARROWSIZE) +
Copy link
Contributor

Choose a reason for hiding this comment

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

OMG math

Copy link
Contributor Author

@emilykl emilykl Jul 22, 2025

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)

@emilykl emilykl marked this pull request as ready for review July 22, 2025 20:57
@emilykl
Copy link
Contributor Author

emilykl commented Jul 22, 2025

@archmoj This is ready for review. A couple questions for you —

  • I had to remove arrayOk: true for showarrow in 23a8f8a due to a failing test . Looking at plot-schema.json it seems arrayOk is not supported for boolean attributes, does that make sense?

  • It would be nice to support showarrow for parcats traces but it looks like all hoverlabel attributes are not supported for parcats, do you know why? I looked into enabling hoverlabel for parcats but it was not straightforward.

@emilykl emilykl changed the title 7278 make triangle optional Make hover label triangle optional (#7278) Jul 24, 2025
@emilykl emilykl changed the title Make hover label triangle optional (#7278) feat: Make hover label triangle optional Jul 25, 2025
@archmoj
Copy link
Contributor

archmoj commented Jul 25, 2025

  • I had to remove arrayOk: true for showarrow in 23a8f8a due to a failing test . Looking at plot-schema.json it seems arrayOk is not supported for boolean attributes, does that make sense?

One could possibly add arrayOk to the otherOpts here:

otherOpts: ['dflt'],

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.

@archmoj
Copy link
Contributor

archmoj commented Jul 25, 2025

  • It would be nice to support showarrow for parcats traces but it looks like all hoverlabel attributes are not supported for parcats, do you know why? I looked into enabling hoverlabel for parcats but it was not straightforward.

We could handle this option for parcats as follow:
Add

hoverItems.trace._hoverlabel = {showarrow: false};

before calling Fx.loneHover in parcats.js.

Then make the following change

var showArrow = (d.trace._hoverlabel || d.trace.hoverlabel)?.showarrow;

@archmoj
Copy link
Contributor

archmoj commented Jul 25, 2025

In addition to parcats trace, we should handle this option in sankey trace.

@emilykl
Copy link
Contributor Author

emilykl commented Jul 25, 2025

  • It would be nice to support showarrow for parcats traces but it looks like all hoverlabel attributes are not supported for parcats, do you know why? I looked into enabling hoverlabel for parcats but it was not straightforward.

We could handle this option for parcats as follow: Add

hoverItems.trace._hoverlabel = {showarrow: false};

before calling Fx.loneHover in parcats.js.

Then make the following change

var showArrow = (d.trace._hoverlabel || d.trace.hoverlabel)?.showarrow;

Oh that's clever, I like that. Maybe for another PR. It would be cool too if we could just support all the hoverlabel options in parcats but not sure if there's a blocker for that.

@archmoj
Copy link
Contributor

archmoj commented Jul 25, 2025

Instead of the solution proposed above,
you could directly look into gd._fullLayout.hoverlabel.showarrow inside loneHover and pass it through.

exports.loneHover = function loneHover(hoverItems, opts) {
...
var gd = opts.gd;

Copy link
Contributor

@camdecoster camdecoster left a 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.

Comment on lines +1930 to +1931
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';
Copy link
Contributor

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?

Suggested change
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`;

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. Slower in Chrome but not in Safari.
Screenshot 2025-07-25 at 4 50 20 PM
Screenshot 2025-07-25 at 4 48 45 PM

Copy link
Contributor

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.

emilykl and others added 2 commits July 25, 2025 16:47
Co-authored-by: Cameron DeCoster <cameron.decoster@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new P1 needed for current cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make hover box caret/triangle optional
4 participants