implement hoverinfo 'none' and 'skip' in sankey #3096
Conversation
antoinerg
requested review from
alexcjohnson and
etpinard
and removed request for
alexcjohnson
|
Note that right now with I figured that if a user doesn't want any highlighting, he/she can use |
|
alternate proposal, before we create some working |
|
Thank you for your help @alexcjohnson :) I think that commit 644c4e3 makes attribute coercion more efficient as you suggested doing! |
|
@antoinerg this looks great now! #3096 (comment) would be lovely but nonblocking - otherwise 💃 |
|
Ok awesome! I'll just wait for the test to complete and merge it :) |
| 'rgba(255, 255, 255, 0.6)' : | ||
| 'rgba(0, 0, 0, 0.2)'; | ||
|
|
||
| coerceLink('color', linkOut.value.map(function() { |
There was a problem hiding this comment.
For the next time you're working in these files @antoinerg, here's Lib.repeat:
Lines 167 to 181 in 7ae6fd6
There was a problem hiding this comment.
Good to know about the existence Lib.repeat! I just mindlessly reused the .map iterator that was already there! I should know better, for loops are much faster than map 🐎 !
| description: 'The links of the Sankey plot.' | ||
| } | ||
| }, 'calc', 'nested'); | ||
| // hide unsupported top-level properties from plot-schema | ||
| attrs.hoverinfo = undefined; |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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).
| description: 'The links of the Sankey plot.' | ||
| } | ||
| }, 'calc', 'nested'); | ||
| // hide unsupported top-level properties from plot-schema | ||
| attrs.hoverinfo = undefined; | ||
| attrs.hoverlabel = undefined; |
There was a problem hiding this comment.
But wait, trace hoverlabel was working before this. Hmm, I'd call this a breaking change.
There was a problem hiding this comment.
... I think we should either make node/link hoverlabel inherit from trace hoverlabel or add some cleanData logic.
There was a problem hiding this comment.
@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 :)
There was a problem hiding this comment.
Ah indeed - good catch @etpinard - I'd go for:
make node/link
hoverlabelinherit from tracehoverlabel
That would be more friendly for the (probably most common) case that users want node and link hover labels styled the same.
There was a problem hiding this comment.
... and maybe we should have a trace hoverinfo that acts similarly.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Closes #2782