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.

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

implement hoverinfo 'none' and 'skip' in sankey #3096

Merged
merged 18 commits into from
Oct 23, 2018
Merged

implement hoverinfo 'none' and 'skip' in sankey #3096

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
69c11fe
implement hoverinfo 'none' and 'skip' in sankey
antoinerg Oct 11, 2018
880973a
sankey node and link each have their own hover attributes
antoinerg Oct 12, 2018
d247b8b
sankey remove unused value
antoinerg Oct 12, 2018
1919b72
fix sankey attributes definition
antoinerg Oct 12, 2018
e942bc5
fix sankey attribute to pass test-image
antoinerg Oct 12, 2018
1813d6c
sankey hide unsupported top-level properties from plot-schema
antoinerg Oct 12, 2018
9e4f1a0
hide unsupported properties in sankey more safely
antoinerg Oct 12, 2018
706e7ae
fix sankey attributes and add better test
antoinerg Oct 15, 2018
7c24d29
fix sankey attributes to pass plotschema test
antoinerg Oct 15, 2018
53d7ce3
fix lint issue
antoinerg Oct 15, 2018
87c30fe
fix sankey attribute key valtype -> valType
antoinerg Oct 15, 2018
7a32036
update description of hoverinfo in sankey
antoinerg Oct 15, 2018
b436003
remove editType on hoverinfo since it's overriden
antoinerg Oct 15, 2018
fe7ab49
sankey update defaults
antoinerg Oct 15, 2018
644c4e3
sankey improve attribute coercion
antoinerg Oct 15, 2018
ccaef04
test sankey hoverlabels defined in template
antoinerg Oct 16, 2018
dc8ba12
sankey move defaultLinkColor outside for loop to speed up coerce
antoinerg Oct 23, 2018
ea73c64
fix typo: defautLinkColor -> defaultLinkColor
antoinerg Oct 23, 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
16 changes: 9 additions & 7 deletions src/traces/sankey/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,7 @@ var domainAttrs = require('../../plots/domain').attributes;
var extendFlat = require('../../lib/extend').extendFlat;
var overrideAll = require('../../plot_api/edit_types').overrideAll;

module.exports = overrideAll({
hoverinfo: extendFlat({}, plotAttrs.hoverinfo, {
flags: ['label', 'text', 'value', 'percent', 'name'],
}),
hoverlabel: fxAttrs.hoverlabel, // needs editType override

var attrs = module.exports = overrideAll({
domain: domainAttrs({name: 'sankey', trace: true}),

orientation: {
Expand Down Expand Up @@ -127,6 +122,8 @@ module.exports = overrideAll({
role: 'style',
description: 'Sets the thickness (in px) of the `nodes`.'
},
hoverinfo: extendFlat({}, plotAttrs.hoverinfo, {flags: []}),
alexcjohnson marked this conversation as resolved.
Show resolved Hide resolved
hoverlabel: fxAttrs.hoverlabel, // needs editType override,
description: 'The nodes of the Sankey plot.'
},

Expand Down Expand Up @@ -185,6 +182,11 @@ module.exports = overrideAll({
role: 'info',
description: 'A numeric value representing the flow volume value.'
},
hoverinfo: extendFlat({}, plotAttrs.hoverinfo, {flags: []}),
hoverlabel: fxAttrs.hoverlabel, // needs editType override,
description: 'The links of the Sankey plot.'
}
},
antoinerg marked this conversation as resolved.
Show resolved Hide resolved
}, 'calc', 'nested');
// hide unsupported top-level properties from plot-schema
attrs.hoverinfo = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh so, trace-level hoverinfo is no longer available for sankey traces? Luckily it never worked so this isn't breaking change.

Does this PR close #3097 ?

Copy link
Contributor Author

@antoinerg antoinerg Oct 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It wouldn't close it.

Issue #3097 is about supporting hoverinfo options other than the basic skip and none. Although the current doc say that we support many:

Any combination of "label", "text", "value", "percent", "name" joined with a "+" OR "all" or "none" or "skip"

none actually worked except all. This PR is solely about adding support for skip and none. Those two are arguably more urgent to support (ie. whether to hide the hoverlabels and whether to emit hover events at the same time).

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to know. Thanks!

attrs.hoverlabel = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

But wait, trace hoverlabel was working before this. Hmm, I'd call this a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

... I think we should either make node/link hoverlabel inherit from trace hoverlabel or add some cleanData logic.

Copy link
Contributor Author

@antoinerg antoinerg Oct 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@etpinard It's great that you noticed that change and I agree this should be handled better!

FYI, having a hover(label|info) in each node and link was the fruit of discussions with @alexcjohnson. In the future, we could add a hovertemplate on each to provide control over their content. That's one way of dealing with point 2 of this comment #3126 (comment) which we can discuss about later on :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah indeed - good catch @etpinard - I'd go for:

make node/link hoverlabel inherit from trace hoverlabel

That would be more friendly for the (probably most common) case that users want node and link hover labels styled the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

... and maybe we should have a trace hoverinfo that acts similarly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hoverinfo is trickier - could be nice to inherit skip/none, but when we get around to supporting a real flaglist, links and nodes will have completely different options.

Copy link
Contributor

Choose a reason for hiding this comment

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

e.g. trace.hoverinfo: 'none' would be equivalent to node.hoverinfo: 'none' + link.hoverinfo: 'none'.

Not all the flags to be implemented in #3097 should be available from the trace hoverinfo, but I think setting trace-wide 'none' and 'skip' could be useful.

11 changes: 11 additions & 0 deletions src/traces/sankey/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ var attributes = require('./attributes');
var Color = require('../../components/color');
var tinycolor = require('tinycolor2');
var handleDomainDefaults = require('../../plots/domain').defaults;
var Registry = require('../../registry');

module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout) {
function coerce(attr, dflt) {
Expand All @@ -24,6 +25,11 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
coerce('node.thickness');
coerce('node.line.color');
coerce('node.line.width');
coerce('node.hoverinfo');
Registry.getComponentMethod(
alexcjohnson marked this conversation as resolved.
Show resolved Hide resolved
'fx',
'supplyDefaults'
)(traceIn.node, traceOut.node, defaultColor, layout);

var colors = layout.colorway;

Expand All @@ -39,6 +45,11 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
coerce('link.value');
coerce('link.line.color');
coerce('link.line.width');
coerce('link.hoverinfo');
Registry.getComponentMethod(
'fx',
'supplyDefaults'
)(traceIn.link, traceOut.link, defaultColor, layout);

coerce('link.color', traceOut.link.value.map(function() {
return tinycolor(layout.paper_bgcolor).getLuminance() < 0.333 ?
Expand Down
68 changes: 40 additions & 28 deletions src/traces/sankey/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,13 @@ 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,
points: [d.link]
});
if(d.link.trace.link.hoverinfo !== 'skip') {
gd.emit('plotly_hover', {
event: d3.event,
points: [d.link]
});
}

};

var sourceLabel = _(gd, 'source:') + ' ';
Expand All @@ -145,7 +148,8 @@ module.exports = function plot(gd, calcData) {

var linkHoverFollow = function(element, d) {
if(gd._fullLayout.hovermode === false) return;
var trace = d.link.trace;
var obj = d.link.trace.link;
if(obj.hoverinfo === 'none' || obj.hoverinfo === 'skip') return;
var rootBBox = gd._fullLayout._paperdiv.node().getBoundingClientRect();
var boundingBox = element.getBoundingClientRect();
var hoverCenterX = boundingBox.left + boundingBox.width / 2;
Expand All @@ -160,11 +164,11 @@ module.exports = function plot(gd, calcData) {
sourceLabel + d.link.source.label,
targetLabel + d.link.target.label
].filter(renderableValuePresent).join('<br>'),
color: castHoverOption(trace, 'bgcolor') || Color.addOpacity(d.tinyColorHue, 1),
borderColor: castHoverOption(trace, 'bordercolor'),
fontFamily: castHoverOption(trace, 'font.family'),
fontSize: castHoverOption(trace, 'font.size'),
fontColor: castHoverOption(trace, 'font.color'),
color: castHoverOption(obj, 'bgcolor') || Color.addOpacity(d.tinyColorHue, 1),
borderColor: castHoverOption(obj, 'bordercolor'),
fontFamily: castHoverOption(obj, 'font.family'),
fontSize: castHoverOption(obj, 'font.size'),
fontColor: castHoverOption(obj, 'font.color'),
idealAlign: d3.event.x < hoverCenterX ? 'right' : 'left'
}, {
container: fullLayout._hoverlayer.node(),
Expand All @@ -179,10 +183,12 @@ 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,
points: [d.link]
});
if(d.link.trace.link.hoverinfo !== 'skip') {
gd.emit('plotly_unhover', {
event: d3.event,
points: [d.link]
});
}

Fx.loneUnhover(fullLayout._hoverlayer.node());
};
Expand All @@ -198,15 +204,19 @@ 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,
points: [d.node]
});
if(d.node.trace.node.hoverinfo !== 'skip') {
gd.emit('plotly_hover', {
event: d3.event,
points: [d.node]
});
}
};

var nodeHoverFollow = function(element, d) {
if(gd._fullLayout.hovermode === false) return;
var trace = d.node.trace;

var obj = d.node.trace.node;
if(obj.hoverinfo === 'none' || obj.hoverinfo === 'skip') return;
var nodeRect = d3.select(element).select('.' + cn.nodeRect);
var rootBBox = gd._fullLayout._paperdiv.node().getBoundingClientRect();
var boundingBox = nodeRect.node().getBoundingClientRect();
Expand All @@ -224,11 +234,11 @@ module.exports = function plot(gd, calcData) {
incomingLabel + d.node.targetLinks.length,
outgoingLabel + d.node.sourceLinks.length
].filter(renderableValuePresent).join('<br>'),
color: castHoverOption(trace, 'bgcolor') || d.tinyColorHue,
borderColor: castHoverOption(trace, 'bordercolor'),
fontFamily: castHoverOption(trace, 'font.family'),
fontSize: castHoverOption(trace, 'font.size'),
fontColor: castHoverOption(trace, 'font.color'),
color: castHoverOption(obj, 'bgcolor') || d.tinyColorHue,
borderColor: castHoverOption(obj, 'bordercolor'),
fontFamily: castHoverOption(obj, 'font.family'),
fontSize: castHoverOption(obj, 'font.size'),
fontColor: castHoverOption(obj, 'font.color'),
idealAlign: 'left'
}, {
container: fullLayout._hoverlayer.node(),
Expand All @@ -243,10 +253,12 @@ 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,
points: [d.node]
});
if(d.node.trace.node.hoverinfo !== 'skip') {
gd.emit('plotly_unhover', {
event: d3.event,
points: [d.node]
});
}

Fx.loneUnhover(fullLayout._hoverlayer.node());
};
Expand Down
111 changes: 88 additions & 23 deletions test/jasmine/tests/sankey_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,9 @@ describe('sankey tests', function() {
Lib.clearThrottle();
}

var node = [404, 302],
link = [450, 300];

it('should show the correct hover labels', function(done) {
var gd = createGraphDiv();
var mockCopy = Lib.extendDeep({}, mock);
Expand Down Expand Up @@ -433,10 +436,14 @@ describe('sankey tests', function() {
);

return Plotly.restyle(gd, {
'hoverlabel.bgcolor': 'red',
'hoverlabel.bordercolor': 'blue',
'hoverlabel.font.size': 20,
'hoverlabel.font.color': 'black'
'node.hoverlabel.bgcolor': 'red',
'node.hoverlabel.bordercolor': 'blue',
'node.hoverlabel.font.size': 20,
'node.hoverlabel.font.color': 'black',
'link.hoverlabel.bgcolor': 'red',
'link.hoverlabel.bordercolor': 'blue',
'link.hoverlabel.font.size': 20,
'link.hoverlabel.font.color': 'black'
antoinerg marked this conversation as resolved.
Show resolved Hide resolved
});
})
.then(function() {
Expand All @@ -459,7 +466,7 @@ describe('sankey tests', function() {
.then(done);
});

it('should show correct hover labels even if there is no link.label supplied', function(done) {
it('should show the correct hover labels even if there is no link.label supplied', function(done) {
var gd = createGraphDiv();
var mockCopy = Lib.extendDeep({}, mock);
delete mockCopy.data[0].link.label;
Expand All @@ -477,16 +484,64 @@ describe('sankey tests', function() {
.then(done);
});

it('should not show labels if hovermode is false', function(done) {
it('should not show any 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);
_hover(node[0], node[1]);
assertNoLabel();
})
.then(function() {
_hover(link[0], link[1]);
assertNoLabel();
})
.catch(failTest)
.then(done);
});

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

Plotly.plot(gd, mockCopy).then(function() {
return Plotly.restyle(gd, 'node.hoverinfo', 'none');
})
.then(function() {
_hover(node[0], node[1]);
assertNoLabel();
})
.catch(failTest)
.then(done);
});

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

Plotly.plot(gd, mockCopy).then(function() {
return Plotly.restyle(gd, 'link.hoverinfo', 'none');
})
.then(function() {
_hover(link[0], link[1]);
assertNoLabel();
})
.catch(failTest)
.then(done);
});

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

Plotly.plot(gd, mockCopy).then(function() {
return Plotly.restyle(gd, 'node.hoverinfo', 'skip');
})
.then(function() {
_hover(node[0], node[1]);
assertNoLabel();
})
.catch(failTest)
Expand Down Expand Up @@ -574,6 +629,7 @@ describe('sankey tests', function() {
var fig = Lib.extendDeep({}, mock);

Plotly.plot(gd, fig)
.then(function() { return Plotly.restyle(gd, 'hoverinfo', 'none'); })
.then(function() { return _hover('node'); })
.then(function(d) {
_assert(d, {
Expand Down Expand Up @@ -610,27 +666,36 @@ describe('sankey tests', function() {
.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!');
})
.then(function() { return _unhover('node'); })
.then(failTest).catch(function(err) {
expect(err).toBe('plotly_unhover did not get called!');
})
.then(function() { return _hover('link'); })
function assertNoHoverEvents(type) {
return Promise.resolve()
.then(function() { return _hover(type); })
.then(failTest).catch(function(err) {
expect(err).toBe('plotly_hover did not get called!');
})
.then(function() { return _unhover('link'); })
.then(function() { return _unhover(type); })
.then(failTest).catch(function(err) {
expect(err).toBe('plotly_unhover did not get called!');
})
});
}

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 assertNoHoverEvents('node');})
.then(function() { return assertNoHoverEvents('link');})
antoinerg marked this conversation as resolved.
Show resolved Hide resolved
.catch(failTest)
.then(done);
});

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

Plotly.plot(gd, fig)
.then(function() { return Plotly.restyle(gd, 'link.hoverinfo', 'skip'); })
.then(function() { return assertNoHoverEvents('link');})
.catch(failTest)
.then(done);
});
});
Expand Down