Skip to content

disable hover for sankey traces when hovermode is false #2949

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 5 commits into from
Aug 29, 2018
Merged

disable hover for sankey traces when hovermode is false #2949

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
edc20b0
disable hover when hovermode is false
antoinerg Aug 28, 2018
45f06c4
sankey test that labels are not shown when hovermode is false
antoinerg Aug 28, 2018
98c18f8
clean up sankey test
antoinerg Aug 28, 2018
aafa886
bail out of the unhover handlers if hovermode is false
antoinerg Aug 28, 2018
3d32b6f
sankey test: adding a hovermode: false case to the click/hover events
antoinerg Aug 28, 2018
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
4 changes: 4 additions & 0 deletions src/traces/sankey/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ module.exports = function plot(gd, calcData) {
};

var linkHover = function(element, d, sankey) {
if(gd._fullLayout.hovermode === false) return;
d3.select(element).call(linkHoveredStyle.bind(0, d, sankey, true));
gd.emit('plotly_hover', {
event: d3.event,
Expand All @@ -143,6 +144,7 @@ module.exports = function plot(gd, calcData) {
var outgoingLabel = _(gd, 'outgoing flow count:') + ' ';

var linkHoverFollow = function(element, d) {
if(gd._fullLayout.hovermode === false) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to bail out of the unhover handlers as well, to ensure that we don't emit plotly_unhover events? Or does this already avoid these events?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcjohnson It was indeed emitting a bunch of plotly_unhover events. It is fixed in my next commit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great! I guess then we should 🔒 the events too, by adding a hovermode: false case to the click/hover events test

var trace = d.link.trace;
var rootBBox = gd._fullLayout._paperdiv.node().getBoundingClientRect();
var boundingBox = element.getBoundingClientRect();
Expand Down Expand Up @@ -193,6 +195,7 @@ module.exports = function plot(gd, calcData) {
};

var nodeHover = function(element, d, sankey) {
if(gd._fullLayout.hovermode === false) return;
d3.select(element).call(nodeHoveredStyle, d, sankey);
gd.emit('plotly_hover', {
event: d3.event,
Expand All @@ -201,6 +204,7 @@ module.exports = function plot(gd, calcData) {
};

var nodeHoverFollow = function(element, d) {
if(gd._fullLayout.hovermode === false) return;
var trace = d.node.trace;
var nodeRect = d3.select(element).select('.' + cn.nodeRect);
var rootBBox = gd._fullLayout._paperdiv.node().getBoundingClientRect();
Expand Down
27 changes: 27 additions & 0 deletions test/jasmine/tests/sankey_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,28 @@ describe('sankey tests', function() {
.catch(failTest)
.then(done);
});

it('should not show labels if hovermode is false', function(done) {
var gd = createGraphDiv();
var mockCopy = Lib.extendDeep({}, mock);

function _hover(px, py) {
mouseEvent('mousemove', px, py);
mouseEvent('mouseover', px, py);
Lib.clearThrottle();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we push this to an outer scope so we're not just repeating the function from above?

Copy link
Collaborator

Choose a reason for hiding this comment

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

heh I see, it was already duplicated - great, we can clean them all up into just one _hover


Plotly.plot(gd, mockCopy).then(function() {
return Plotly.relayout(gd, 'hovermode', false);
})
.then(function() {
_hover(404, 302);

assertNoLabel();
})
.catch(failTest)
.then(done);
});
});

describe('Test hover/click event data:', function() {
Expand Down Expand Up @@ -620,3 +642,8 @@ function assertLabel(content, style) {
fontColor: style[4]
});
}

function assertNoLabel() {
var g = d3.selectAll('.hovertext');
expect(g[0].length).toBe(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

g.size() is better than g[0].length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

}