disable hover for sankey traces when hovermode is false #2949
Conversation
test/jasmine/tests/sankey_test.js
Outdated
|
|
||
| function assertNoLabel() { | ||
| var g = d3.selectAll('.hovertext'); | ||
| expect(g[0].length).toBe(0); |
There was a problem hiding this comment.
g.size() is better than g[0].length
test/jasmine/tests/sankey_test.js
Outdated
| mouseEvent('mousemove', px, py); | ||
| mouseEvent('mouseover', px, py); | ||
| Lib.clearThrottle(); | ||
| } |
There was a problem hiding this comment.
Can we push this to an outer scope so we're not just repeating the function from above?
There was a problem hiding this comment.
heh I see, it was already duplicated - great, we can clean them all up into just one _hover
push _hover into outer scope. use d3 size() instead of manually checking objects length.
| @@ -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; | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@alexcjohnson It was indeed emitting a bunch of plotly_unhover events. It is fixed in my next commit.
There was a problem hiding this comment.
Great! I guess then we should 🔒 the events too, by adding a hovermode: false case to the click/hover events test
| .then(function() { return _hover('node'); }) | ||
| .then(failTest).catch(function(err) { | ||
| expect(err).toBe('plotly_hover did not get called!'); | ||
| }) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(no it doesn't give a line number, but neither do our normal failTest invocations... lets leave it as is at least for now)
|
Nicely done @antoinerg! 💃 |
Fixes #2782
Simple fix: hover handler functions now simply return if hovermode is false.
cc @etpinard