Skip to content
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

fix: do not ovelap hoverlabels for different tracetypes #6954

Merged
merged 6 commits into from
May 30, 2024

Conversation

mbant
Copy link
Contributor

@mbant mbant commented Apr 10, 2024

This PR attempts a fix for #6917 which reported excessive hoverlabel removal and overlap for plots with both scatter and bar traces.

Behaviour before the fix:

Screenshot 2024-04-10 at 21 54 14

or even

Screenshot 2024-04-10 at 21 56 01

Behaviour after the fix:

Screenshot 2024-04-10 at 21 51 35
JSON file used for the example
{
  "data": [
    {
      "name": "scatter_1",
      "type": "scatter",
      "y": [0, 1, 2],
      "x": ["CAT 1", "CAT 2", "CAT 3"]
    },
    {
      "name": "bar_1",
      "type": "bar",
      "y": [0, 1, 2],
      "x": ["CAT 1", "CAT 2", "CAT 3"]
    },
    {
      "name": "scatter_2",
      "type": "scatter",
      "x": ["CAT 1", "CAT 2", "CAT 3"],
      "y": [0, 1, 2]
    },
    {
      "name": "bar_2",
      "type": "bar",
      "y": [0, 1, 2],
      "x": ["CAT 1", "CAT 2", "CAT 3"]
    },
    {
      "name": "scatter_3",
      "type": "scatter",
      "x": ["CAT 1", "CAT 2", "CAT 3"],
      "y": [0, 1, 2]
    },
    {
      "name": "bar_3",
      "type": "bar",
      "y": [0, 1, 2],
      "x": ["CAT 1", "CAT 2", "CAT 3"]
    }
  ],
  "layout": {
    "width": 600,
    "height": 400,
    "hovermode": "x"
  }
}

@mbant
Copy link
Contributor Author

mbant commented May 29, 2024

#6917 (comment)

@archmoj / @alexcjohnson any reason why this can't be considered yet?

@archmoj
Copy link
Contributor

archmoj commented May 29, 2024

Thanks very much for the PR.
It is looking good on my side.

@archmoj archmoj requested a review from alexcjohnson May 29, 2024 12:37
@@ -1757,7 +1757,7 @@ function hoverAvoidOverlaps(hoverLabels, rotateLabels, fullLayout, commonLabelBo
topOverlap = p0.pos + p0.dp + p0.size - p1.pos - p1.dp + p1.size;

// Only group points that lie on the same axes
if(topOverlap > 0.01 && (p0.pmin === p1.pmin) && (p0.pmax === p1.pmax)) {
if(topOverlap > 0.01 && p0.crossAxKey === p1.crossAxKey) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand this actually - I only see us using crossAxKey as a variable name, not a key in these point objects.

Given that this change fixes the behavior, you've clearly identified that this condition is the one causing this bug, but AFAICT the change is equivalent to just if(topOverlap > 0.01) - and maybe that's right? Maybe the pmin/pmax conditions are no longer useful (which presumably means the comment Only group points that lie on the same axes is also no longer what we want) but that's the question to answer before we move forward with this.

@archmoj I've forgotten the details of this, and anyway it changed recently with the hoversubplots work, but if you can convince yourself that removing this condition entirely is correct in all the cases we care about (in addition to the case being tested here, I'd focus on overlaid subplots with points at the same position on screen but either different x axis, different y axis, or both) then go for it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100% agreed on the condition. It's been a while so not sure why that was there ...

I pushed a commit to simplify the condition (and thus removing the comment) if that helps with testing / convincing; I will have time to pick this up again in the following days so @archmoj let me know if I can help with anything!

@archmoj
Copy link
Contributor

archmoj commented May 30, 2024

It would be nice to test hoversubplots cases.
@mbant Would you please fetch upstream/master and merge it into this branch?
Thank you!

@archmoj
Copy link
Contributor

archmoj commented May 30, 2024

Excellent bug fix.
💃

@archmoj archmoj merged commit 7ffcdd7 into plotly:master May 30, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants