Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

fix: do not ovelap hoverlabels for different tracetypes #6954

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
May 30, 2024
Merged

fix: do not ovelap hoverlabels for different tracetypes #6954

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8d1a49e
fix: do not ovelap hoverlabels for different tracetypes
mbant Apr 10, 2024
74dc83d
chore: draftlog
mbant Apr 10, 2024
6cfc094
fix: var instead of let
mbant Apr 10, 2024
33b28ec
fix: fix eslint mismatch
mbant Apr 10, 2024
4321451
fix: simplify label grouping condition
mbant May 30, 2024
0631d43
Merge branch 'plotly:master' into issue-6917
mbant May 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
fix: do not ovelap hoverlabels for different tracetypes
  • Loading branch information
@mbant
mbant committed Apr 10, 2024
commit 8d1a49eb7020becbd0bb91864f043c5603b6b08c
2 changes: 1 addition & 1 deletion 2 src/components/fx/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@alexcjohnson alexcjohnson May 29, 2024 •
edited
Loading

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!

// push the new point(s) added to this group out of the way
for(j = g1.length - 1; j >= 0; j--) g1[j].dp += topOverlap;

Expand Down
57 changes: 57 additions & 0 deletions 57 test/jasmine/tests/hover_label_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1668,6 +1668,10 @@ describe('hover info', function() {
return Math.max(0, overlap);
}

function labelCount() {
return d3Select(gd).selectAll('g.hovertext').size();
}

it('centered-aligned, should render labels inside boxes', function(done) {
var trace1 = {
x: ['giraffes'],
Expand Down Expand Up @@ -1787,6 +1791,59 @@ describe('hover info', function() {
})
.then(done, done.fail);
});

it("does not overlap lebels for different trace types", function (done) {
function trace(name, type, delta) {
return {
name: name,
type: type,
y: [0 + delta, 1 + delta, 2 + delta],
x: ["CAT 1", "CAT 2", "CAT 3"],
};
}

var scatterName = "scatter_";
var barName = "bar_";
var data = [];
for(let i = 0; i<3; i++) {
data.push(trace(barName + i, "bar", 0.0));
data.push(trace(scatterName + i, "scatter", 0.1));
}
var layout = {
width: 600,
height: 400,
hovermode: "x",
};

Plotly.newPlot(gd, data, layout)
.then(function () {
_hoverNatural(gd, 200, 200);
})
.then(function () {
expect(labelCount()).toBe(6);
})
.then(function () {

var nodes = [];
for(let i = 0; i<3; i++) {
nodes.push(hoverInfoNodes(barName + i).secondaryBox.getBoundingClientRect());
nodes.push(hoverInfoNodes(scatterName + i).secondaryBox.getBoundingClientRect());
}
nodes.sort(function(a,b) { return a.top - b.top; } );

for(let i = 0; i<5; i++) {
expect(
calcLineOverlap(
nodes[i].top,
nodes[i].bottom,
nodes[i+1].top,
nodes[i+1].bottom,
)
).toBeWithin(2, 1);
}
})
.then(done, done.fail);
});
});

describe('constraints info graph viewport', function() {
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.