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
antoinerg 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 all 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
6 changes: 6 additions & 0 deletions src/traces/sankey/plot.js
View file
Open in desktop
Original file line numberDiff line numberDiff 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;

@alexcjohnsonalexcjohnsonAug 28, 2018

Copy link
Copy Markdown
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?

@antoinergantoinergAug 28, 2018

Copy link
Copy Markdown
ContributorAuthor

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.

@alexcjohnsonalexcjohnsonAug 28, 2018

Copy link
Copy Markdown
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 DownExpand Up@@ -175,6 +177,7 @@ module.exports = function plot(gd, calcData) {
};

var linkUnhover = function(element, d, sankey) {
if(gd._fullLayout.hovermode === false) return;
d3.select(element).call(linkNonHoveredStyle.bind(0, d, sankey, true));
gd.emit('plotly_unhover', {
event: d3.event,
Expand All@@ -193,6 +196,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 +205,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 DownExpand Up@@ -236,6 +241,7 @@ module.exports = function plot(gd, calcData) {
};

var nodeUnhover = function(element, d, sankey) {
if(gd._fullLayout.hovermode === false) return;
d3.select(element).call(nodeNonHoveredStyle, d, sankey);
gd.emit('plotly_unhover', {
event: d3.event,
Expand Down
97 changes: 72 additions & 25 deletions test/jasmine/tests/sankey_test.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -388,16 +388,16 @@ describe('sankey tests', function() {
describe('Test hover/click interactions:', function() {
afterEach(destroyGraphDiv);

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

it('should show the correct hover labels', function(done) {
var gd = createGraphDiv();
var mockCopy = Lib.extendDeep({}, mock);

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

Plotly.plot(gd, mockCopy).then(function() {
_hover(404, 302);

Expand DownExpand Up@@ -464,12 +464,6 @@ describe('sankey tests', function() {
var mockCopy = Lib.extendDeep({}, mock);
delete mockCopy.data[0].link.label;

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

Plotly.plot(gd, mockCopy)
.then(function() {
_hover(450, 300);
Expand All@@ -482,6 +476,22 @@ 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);

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 DownExpand Up@@ -527,44 +537,52 @@ describe('sankey tests', function() {
mouseEvent('mouseout', pos[0], pos[1]);
});

it('should output correct hover/click/unhover event data', function(done) {
var fig = Lib.extendDeep({}, mock);
function _assert(d, expectedPtData) {
expect(d.event).toBeDefined('original event reference');

function _assert(d, expectedPtData) {
expect(d.event).toBeDefined('original event reference');
var ptData = d.points[0];
Object.keys(expectedPtData).forEach(function(k) {
expect(ptData[k]).toBe(expectedPtData[k], 'point data for ' + k);
});
}

var ptData = d.points[0];
Object.keys(expectedPtData).forEach(function(k) {
expect(ptData[k]).toBe(expectedPtData[k], 'point data for ' + k);
});
}
it('should output correct click event data', function(done) {
var fig = Lib.extendDeep({}, mock);

Plotly.plot(gd, fig)
.then(function() { return _hover('node'); })
.then(function() { return _click('node'); })
.then(function(d) {
_assert(d, {
curveNumber: 0,
pointNumber: 4,
label: 'Solid'
});
})
.then(function() { return _hover('link'); })
.then(function() { return _click('link'); })
.then(function(d) {
_assert(d, {
curveNumber: 0,
pointNumber: 61,
value: 46.477
});
})
.then(function() { return _click('node'); })
.catch(failTest)
.then(done);
});

it('should output correct hover/unhover event data', function(done) {
var fig = Lib.extendDeep({}, mock);

Plotly.plot(gd, fig)
.then(function() { return _hover('node'); })
.then(function(d) {
_assert(d, {
curveNumber: 0,
pointNumber: 4,
label: 'Solid'
});
})
.then(function() { return _click('link'); })
.then(function() { return _hover('link'); })
.then(function(d) {
_assert(d, {
curveNumber: 0,
Expand All@@ -591,6 +609,30 @@ describe('sankey tests', function() {
.catch(failTest)
.then(done);
});

it('should not output hover/unhover event data when hovermoder is false', function(done) {
var fig = Lib.extendDeep({}, mock);

Plotly.plot(gd, fig)
.then(function() { return Plotly.relayout(gd, 'hovermode', false); })
.then(function() { return _hover('node'); })
.then(failTest).catch(function(err) {
expect(err).toBe('plotly_hover did not get called!');
})

@alexcjohnsonalexcjohnsonAug 28, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, clever 🎉
Does failTest give an intelligible message if we get there? ie if we run this test without your previous commit, would it be clear what went wrong?

@antoinergantoinergAug 28, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It returns: Chrome 68.0.3440 (Windows 10 0.0.0) sankey tests Test hover/click event data: should not output hover/unhover event data when hovermoder is false FAILED
followed by:
Expected Object({ event: [object MouseEvent], points: [ Object({ pointNumber: 4, label: 'Solid', color: 'rgba(148, 103, 189, 0.8)', sourceLinks: [ Object({ pointNumber: 60, label: '', color: 'rgba(0,0,96,0.2)', source: <circular reference: Object>, target: Object({ pointNumber: 26, label: 'Thermal generation', color: 'rgba(227, 119, 194, 0.8)', sourceLinks: [ Object({ pointNumber: 63, label: '', color: 'rgba(0,0,96,0.2)', source: <circular reference: Object>, target: Object, value: 787.129, originalIndex: 63, dy: 84.76807405050613, ty: 0, sy: 0, trace: Object, curveNumber: 0 }), Object({ pointNumber: 62, label: '', color: 'rgba(0,0,96,0.2)', source: <circular reference: Object>, target: Object, value: 525.531, originalIndex: 62, dy: 56.595870211663566, ty: 0, sy: 84.76807405050613, trace: Object, curveNumber: 0 }), Object({ pointNumber: 64, label: '', color: 'rgba(0,0,96,0.2)', source: <circular reference: Object>, target: Object, value: 79.329, originalIndex: 64, dy: 8.543156898491352, ty: 0, ... to be undefined.

What would be preferred?

@alexcjohnsonalexcjohnsonAug 28, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That looks great, assuming there's a line number along with it too, so you can tell it's the _hover on line 618 that caused the failure.

@alexcjohnsonalexcjohnsonAug 29, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(no it doesn't give a line number, but neither do our normal failTest invocations... lets leave it as is at least for now)

.then(function() { return _unhover('node'); })
.then(failTest).catch(function(err) {
expect(err).toBe('plotly_unhover did not get called!');
})
.then(function() { return _hover('link'); })
.then(failTest).catch(function(err) {
expect(err).toBe('plotly_hover did not get called!');
})
.then(function() { return _unhover('link'); })
.then(failTest).catch(function(err) {
expect(err).toBe('plotly_unhover did not get called!');
})
.then(done);
});
});
});

Expand DownExpand Up@@ -620,3 +662,8 @@ function assertLabel(content, style) {
fontColor: style[4]
});
}

function assertNoLabel() {
var g = d3.selectAll('.hovertext');
expect(g.size()).toBe(0);
}