Custom trace hover labels #1582

etpinard · 2017-04-11T18:35:09Z

resolves #102

This PR builds on top of #1573 where annotation hover labels were added with support for custom bg color, border color and font settings. Trace hover labels will be customisable using the same attributes as for annotations, but unlike for annotations, these attributes will be arrayOk: true. For example,

Questions:

should we add layout.?axis.hoverlabel attribute containers to make the the hover label that appear on the x(y) axes on hovermode: 'x'('y') customisable? UPDATE: commits f986fd2 and a8cc8b0 implement layout.hoverlabel
some attribute declarations and logic are duplicated across annotations and trace modules, maybe we should add a new component in e.g. in src/component/hover/? This new component would include graph_interact.js which is becoming less and less a cartesian module. Done in Fx component #1613

TODO:

make sure this works for all trace types
- e.g. fill calcdata accordingly heatmap/calc.js to support 2D data arrays
- add handlers for trace type that don't use Fx.hover (e.g. gl3d. gl2d) on hover

- allow arrayOk for all hover label attribute - don't set dflt for bgcolor, bordercolor and font.color as their value depends and the trace color and plot bgcolor

- to that there (as opposed to in trace module hoverPoints) to have to repeat this logic across all trace modules - note that PR #1573 already added the relevant `createHoverText` logic

etpinard · 2017-04-11T19:36:22Z

cc @alexcjohnson @n-riesco

alexcjohnson · 2017-04-12T03:09:11Z

should we add layout.?axis.hoverlabel attribute containers to make the the hover label that appear on the x(y) axes on hovermode: 'x'('y') customisable?

My gut reaction is to make layout.hoverlabel, ie a default label style for the whole plot, which would set defaults for all the hover labels, including the new annotation labels, but the colors would only be used (as a default) by the common (axis) labels. It's true that this would preclude different styling for the different axes on which these common labels might appear, but offhand that seems like a fairly obscure use case to me, whereas I can definitely see people wanting a concise way to tailor the style of all the labels across the whole chart.

maybe we should add a new component in e.g. in src/component/hover/? This new component would include graph_interact.js which is becoming less and less a cartesian module.

Absolutely. Dunno if now is the time to do it, may be a bit of a project, but if you're excited about it now then by all means go for it.

n-riesco · 2017-04-12T11:24:42Z

should we add layout.?axis.hoverlabel attribute containers to make the the hover label that appear on the x(y) axes on hovermode: 'x'('y') customisable?

I could understand some users wanting the data and the axis labels have the same style.
Neither layout.hoverlabel nor layout.?axis.hoverlabel would help with that.

I can see a case for layout.?axis.hoverlabel in bar charts (so +1 from me).

Also +1 from for layout.hoverlabel, because, as a user, if I can style data labels using data[n].hoverlabel, I'd also expect to be able to style axis labels.

add a new component in e.g. in src/component/hover/

What Alex said (+1 from me too)

etpinard · 2017-04-12T13:40:56Z

OK. I'll add layout.hoverlabel in this PR - where it will set the default hover label settings for trace, common and annotations hover labels (similar to how layout.font impacts other font attributes). For example,

layout: {
  hoverlabel: {
    bgcolor: 'white',
    font: {
      size: 20,
      color: 'black'
    }
  }
}

data = [{
  y: [/* ... */],
  hoverlabel: {
     font: { 
       color: 'biue'
     }
  }
}]

would make all hover labels have a white bg, common labels will have black text and trace labels blue text. Sounds ok?

Then, if someone really wants per-axis common label customizable down the road, we can always add ?axis.hoverlabel later.

n-riesco · 2017-04-12T14:07:43Z

would make all hover labels have a white bg, common labels will have black text and trace labels blue text. Sounds ok?

+1 to white bg
I'd suggest black text as a safer default for all types of labels.

alexcjohnson · 2017-04-12T14:26:30Z

would make all hover labels have a white bg, common labels will have black text and trace labels blue text. Sounds ok?

I was thinking the colors in layout.hoverlabel would only apply to common labels and annotation labels, since the colors in trace labels pull their defaults from a different route and this serves an important purpose disambiguating traces. But other attributes (font family & size, border width) would propagate to traces.

But @n-riesco raises a good point:

I could understand some users wanting the data and the axis labels have the same style.

That would be annoying the way I'm suggesting, as it would require explicitly coloring all the trace labels. I suppose we could make some boolean attribute like layout.hoverlabel.applycolorstotraces for this case, and have it default to false? Dunno, maybe that's too much, but it would have the advantage that you can say "oh, see, the trace labels get their own colors because applycolorstotraces is false."

Then, if someone really wants per-axis common label customizable down the road, we can always add ?axis.hoverlabel later.

👍

n-riesco · 2017-04-12T18:02:26Z

@alexcjohnson @etpinard I really misunderstood layout.?axis.hoverlabel. Please, ignore my comments about it.

Please, correct me if I'm mistaken. This is how understand hoverlabel now:

data[n].hoverlabel is arrayOk: true and sets the style of trace hover labels (I'll call these labels data hover labels)
layout.?axis.hoverlabel is arrayOk: false and would set the style of the hover labels displayed over the ?axis (I'll call these labels axis hover labels).
layout.hoverlabel is arrayOk: false and would set the style of any hover labels (axis, annotations, data).

I was thinking the colors in layout.hoverlabel would only apply to common labels and annotation labels, since the colors in trace labels pull their defaults from a different route and this serves an important purpose disambiguating traces. But other attributes (font family & size, border width) would propagate to traces.

Oh, I missed that at the moment data hover labels are coloured after the marker!

I could understand some users wanting the data and the axis labels have the same style.

When I wrote that, what I had in mind was a bar chart (each bar with different colours). Currently bar charts have axis hover labels with a black background, whereas data hover labels are coloured after the bar. The use case I had in mind was that both axis and data hover labels were coloured after the bar.

How about having layout.hoverlabel.autocolor to support this use case?
When layout.hoverlabel.autocolor is true any hover labels (axis, annotations) will use the same colour as the data hover label (i.e by default the marker colour, unless data[n].hoverlabel is defined).

alexcjohnson · 2017-04-12T18:12:42Z

How about having layout.hoverlabel.autocolor to support this use case?
When layout.hoverlabel.autocolor is true any hover labels (axis, annotations) will use the same colour as the data hover label (i.e by default the marker colour, unless data[n].hoverlabel is defined).

When there's a common label it's because you're in a hover mode (ie compare) that supports multiple labels... so this wouldn't be meaningful in general.

- into component/fx/ - keep old 'init' routine in graph_interact.js - mv hover constants out of cartesian constants.js and into fx/

- this removes one more circular dep!

Fx component

- to coerce hoverlabel container in a DRY way

- somehow `madge` identifies circular in an order-dependent way, bump back to pre PR #1613 value.

etpinard · 2017-04-25T17:59:39Z

tasks/test_syntax.js

@@ -177,7 +177,7 @@ function assertCircularDeps() {
        var logs = [];

        // see https://github.com/plotly/plotly.js/milestone/9
-        var MAX_ALLOWED_CIRCULAR_DEPS = 12;
+        var MAX_ALLOWED_CIRCULAR_DEPS = 17;


I got a little too excited in #1613, that PR didn't remove any circular deps. It only made madge think that there were less ciruclar deps (their algo must order-dependent).

If you don't believe me, try adding var Fx = require('../fx'); in annotations_defaults like here and you get back the 12 circular deps of PR #1613 .

etpinard · 2017-04-25T18:05:19Z

@alexcjohnson this PR is ready for review. It's probably best to review it commit by commit after the #1613 merge commit.

To note, cartesian, geo, ternary and mapbox subplot use Fx.hover while pie, gl3d and gl2d use Fx.loneHover. A lot of redundant logic could be 🔪 by making a more general hover routine that all subplot types could call on even grounds. Moreover, making gl3d and gl2d use gd.calcdata would also help DRY things up 🌴 Oh well, at least our hover test suites are becoming pretty solid 🔒 .

- to test main codepath instead of fallback `ax._length`

- so that e.g. the hover label `color` field isn't overridden by undefined `hoverlabel.bgcolor` values.

- and all (future) plots/attributes.js declarations.

Hover bugs #1575 and #1600

alexcjohnson · 2017-05-09T17:54:00Z

src/components/fx/helpers.js

+
+// convenience functions for mapping all relevant axes
+exports.flat = function flat(subplots, v) {
+    var out = [];


faster (here and in p2c below) to do var out = new Array(subplots.length) and avoid push?

done in e45bff9

alexcjohnson · 2017-05-09T18:22:00Z

src/components/fx/hover.js

-            .style({fill: Color.defaultLine, 'stroke-width': '1px', stroke: Color.background});
+            .style({
+                fill: commonLabelOpts.bgcolor || Color.defaultLine,
+                stroke: commonLabelOpts.bordercolor || Color.background,


might want to be Color.fill and Color.stroke since these I guess support alpha now

I started looking around at other places this might be needed... and there are a bunch potentially (seems like all in this file), including some in spikelines that I didn't catch when reviewing that. I'm not sure if it's really crucial, the distinction is mainly important for export right? And you're not going to export hover effects. Still, best to be consistent.

After some thought, I think it would be best to not render the alpha channel in all hover labels, as the marker hover labels already blend marker opacities with plot_bgcolor filtering out the alpha channel. For example,

var trace = { x: [1], y: [1], marker: { color: ['rgba(255, 0, 0, 0.3)'] } } var layout = { plot_bgcolor: 'grey' }

renders the marker hover label as rgb(166, 90, 90) i.e. with no alpha channel.

We could add some special logic so that e.g.

var trace = { x: [1], y: [1], marker: { color: ['rgba(255, 0, 0, 0.3)'] }, hovelabel: { bgcolor: 'rgba(0,0,255,0.3)' } } var layout = { plot_bgcolor: 'grey' }

renders the alpha channel overriding the marker color to hover label color behavior, but quite frankly I don't see the need.

Adding a line in the schema description saying that alpha channel aren't honored sounds sufficient for now. Thoughts?

I've occasionally thought it would be nice to be able to see behind the hover labels - not just behind the name field like you can do now, but for example in a fractal when I want to draw a small zoom box it would be cool if the whole label were partially transparent. But in cases like that, seems like the best solution would be to make another attribute - hoverlabel.opacity, akin to marker.opacity so you don't need to set the background, the font color, and the border color all to rgba values - in fact, doing it that way would be a substandard experience for exactly the same reason as we have a marker.opacity in the first place, that you really want opacity applied to the whole group.

So yeah, I guess I'm agreeing with you that these attributes needn't support opacity on their own. If you feel like putting in hoverlabel.opacity now go for it, but it can also be left out of this PR and done later.

but it can also be left out of this PR and done later.

I'm going to go with that if you don't mind. This PR is big enough.

alexcjohnson · 2017-05-09T18:30:06Z

src/plots/gl2d/scene2d.js

@@ -667,3 +675,18 @@ proto.hoverFormatter = function(axisName, val) {
    var axis = this[axisName];
    return Axes.tickText(axis, axis.c2l(val), 'hover').text;
 };
+
+proto.castHoverOption = function(trace, ptNumber, attr) {


possible to share this with the one in gl3d? Looks nearly identical?

It is possible. I didn't do it because as brought up in #1582 (comment), the real solution would be to replace Fx.loneHover by something more similar to Fx.hover in gl2d and gl3d. But, for the time being, might as well put this logic something in the Fx module object.

done in 60bd4fc

etpinard · 2017-05-10T16:29:53Z

src/traces/sankey/plot.js

@@ -124,7 +124,8 @@ module.exports = function plot(gd, calcData) {
    };

    var linkHoverFollow = function(element, d) {
-
+        var trace = gd._fullData[d.traceId];
+        var ptNumber = d.originalIndex;


@monfera does this look ok to you? I'm looking for the data array index corresponding to this link / node (below).

Oh wait. node and link can have different lengths, correct? So maybe hoverlabel should be set per-node and per-link e.g.:

var trace = { node: { hoverlabel: { bgcolor: [/* same length as node data arrays */] }, link: { hoverlabel: { bgcolor: [/* same length as link data arrays */] } } }, hoverlabel: { // for trace-wide hover label settings (that would be applied to all nodes and links) } }

Well the above sounds uselessly complicated for v1 of sankey. So, I'd vote for not allowing array values in hoverlabel attribute of sankey traces. Does that sound ok?

Yes your conclusion sounds good to me. Indeed it'll be an unlikely occurrence that the number of nodes and links match.

✅ in e50886c

🔒 in f8258cb

We can leave it off for now to get 1.27 out, but we should definitely include custom hovertext (and at that point we might as well include custom styling) for both nodes and links in the near future - I can definitely see people wanting to explain what specifically is included in each of these elements in more detail than is displayed permanently on the diagram. Chatting with @monfera seems like link.label fills this role for links but there's no analog for nodes. I'll make an issue for this so we don't forget about it.

I'll make an issue for this so we don't forget about it.

Thanks!

- see 4d3ce92#r115788926 for rationale

alexcjohnson · 2017-05-10T17:57:36Z

test/jasmine/tests/sankey_test.js

+            return new Promise(function(resolve) {
+                setTimeout(resolve, 60);
+            });
+        }


https://github.com/plotly/plotly.js/blob/master/test/jasmine/assets/delay.js ?

That thing got 🔪

I've been using this pattern lately. No need for timeouts 🎉

Ah nice 🚀

alexcjohnson · 2017-05-10T17:59:37Z

test/jasmine/tests/sankey_test.js

-
-                        window.setTimeout(function() {
-                            expect(d3.select('.hoverlayer>.hovertext>text').node().innerHTML)
-                                .toEqual('46TWh', 'tooltip jumped to link');


this expect got lost?

it's there: https://github.com/plotly/plotly.js/pull/1582/files#diff-6c72cf011d0c1bbd8d91d759790188aeR330

👍 sorry, missed that!

alexcjohnson · 2017-05-10T18:54:52Z

💃 once tests pass. Epic PR @etpinard 🏆

etpinard added 3 commits April 11, 2017 14:10

add hoverlabel attribute container for all traces

83cd8c8

- allow arrayOk for all hover label attribute - don't set dflt for bgcolor, bordercolor and font.color as their value depends and the trace color and plot bgcolor

merge hoverlabel items in scatter and bar calcdata

258f1c7

add hover label setting to hoverData during cleanPoint

fe50b6f

- to that there (as opposed to in trace module hoverPoints) to have to repeat this logic across all trace modules - note that PR #1573 already added the relevant `createHoverText` logic

etpinard added status: in progress feature something new labels Apr 11, 2017

etpinard added this to the 1.27.0 milestone Apr 18, 2017

etpinard added 6 commits April 20, 2017 09:50

Merge branch 'master' into hoverlabel-custom

8d7ec16

break up cartesian graph_interact.js:

ffde987

- into component/fx/ - keep old 'init' routine in graph_interact.js - mv hover constants out of cartesian constants.js and into fx/

update Fx require statements

1d4930c

update hover constant require statements

e609955

sub Fx.init by (cartesian) initIterations

12954aa

decrease max allowed circular deps to 13 🎉

27fb2d0

etpinard mentioned this pull request Apr 20, 2017

Fx component #1613

Merged

etpinard added 9 commits April 20, 2017 14:52

mv fx attribute out of plots/ and into components/fx/

a96af57

register fx in core.js

abcf1b0

- this removes one more circular dep!

Merge pull request #1613 from plotly/fx-component

03554a5

Fx component

revert hoverlabel in arraysToCalcdata -> add Fx.calc

efcb027

make fx default test more robust

7907140

lint fx constants

0d66e6e

lint getComponentMethod call

3ac4c07

add fx supplyDefaults method

295659e

- to coerce hoverlabel container in a DRY way

fix mapbox hover label color for array marker.color values

9d91e2c

bump back max allowed circular deps to 17

ce27e10

- somehow `madge` identifies circular in an order-dependent way, bump back to pre PR #1613 value.

etpinard commented Apr 25, 2017

View reviewed changes

etpinard added 2 commits April 26, 2017 14:31

resolves #1575 - call Fx.hover with evt object containing 'xpx' & 'ypx'

be9e6b9

- to test main codepath instead of fallback `ax._length`

fixes #1600 - allow z cartesian traces to have a name hover label

cf61dc2

etpinard mentioned this pull request Apr 26, 2017

Hover bugs #1575 and #1600 #1627

Merged

etpinard added 2 commits April 26, 2017 17:52

don't override hover label data by undefined vals

9eeadcf

- so that e.g. the hover label `color` field isn't overridden by undefined `hoverlabel.bgcolor` values.

make sure arrayOk resyle logic works for hoverlabel attributes

8321959

- and all (future) plots/attributes.js declarations.

etpinard mentioned this pull request May 8, 2017

Sankey feature branch #1591

Merged

Merge pull request #1627 from plotly/hover-misc-bugs

409b771

Hover bugs #1575 and #1600

alexcjohnson reviewed May 9, 2017

View reviewed changes

etpinard added 3 commits May 9, 2017 15:53

factor out gl2d / gl3d cast hover options logic into 1 Fx method

60bd4fc

perf improvements in fx helpers

e45bff9

Merge branch 'master' into hoverlabel-custom

e0be6cd

etpinard mentioned this pull request May 10, 2017

Recompute hover on click to increase click robustness #1646

Merged

etpinard added 2 commits May 10, 2017 12:23

🔪 🌲 in sankey tests

8d021e0

implement 'hoverlabel' settings in sankey hover routines

4d3ce92

etpinard commented May 10, 2017

View reviewed changes

etpinard added 2 commits May 10, 2017 13:27

fixup: don't support array value in sankey hoverlabel settings

e50886c

- see 4d3ce92#r115788926 for rationale

add sankey hover label style tests

f8258cb

alexcjohnson mentioned this pull request May 10, 2017

hoverlabel.opacity #1665

Closed

alexcjohnson reviewed May 10, 2017

View reviewed changes

Merge branch 'master' into hoverlabel-custom

3f52a89

etpinard merged commit c53ae17 into master May 10, 2017

etpinard deleted the hoverlabel-custom branch May 10, 2017 18:59

etpinard mentioned this pull request Sep 13, 2017

Allow setting 'hoverformat' for z-values in heatmap #1383

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom trace hover labels #1582

Custom trace hover labels #1582

etpinard commented Apr 11, 2017 •

edited

Loading

etpinard commented Apr 11, 2017

alexcjohnson commented Apr 12, 2017

n-riesco commented Apr 12, 2017

etpinard commented Apr 12, 2017

n-riesco commented Apr 12, 2017

alexcjohnson commented Apr 12, 2017

n-riesco commented Apr 12, 2017 •

edited

Loading

alexcjohnson commented Apr 12, 2017

etpinard Apr 25, 2017

etpinard commented Apr 25, 2017

alexcjohnson May 9, 2017

etpinard May 9, 2017

alexcjohnson May 9, 2017

alexcjohnson May 9, 2017

etpinard May 9, 2017

alexcjohnson May 10, 2017

etpinard May 10, 2017

alexcjohnson May 10, 2017

alexcjohnson May 9, 2017

etpinard May 9, 2017

etpinard May 9, 2017

etpinard May 10, 2017

monfera May 10, 2017

etpinard May 10, 2017 •

edited

Loading

alexcjohnson May 10, 2017

etpinard May 10, 2017

alexcjohnson May 10, 2017

alexcjohnson May 10, 2017

etpinard May 10, 2017

alexcjohnson May 10, 2017

alexcjohnson May 10, 2017

etpinard May 10, 2017

alexcjohnson May 10, 2017

alexcjohnson commented May 10, 2017

Custom trace hover labels #1582

Custom trace hover labels #1582

Conversation

etpinard commented Apr 11, 2017 • edited Loading

etpinard commented Apr 11, 2017

alexcjohnson commented Apr 12, 2017

n-riesco commented Apr 12, 2017

etpinard commented Apr 12, 2017

n-riesco commented Apr 12, 2017

alexcjohnson commented Apr 12, 2017

n-riesco commented Apr 12, 2017 • edited Loading

alexcjohnson commented Apr 12, 2017

Choose a reason for hiding this comment

etpinard commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented May 10, 2017

etpinard commented Apr 11, 2017 •

edited

Loading

n-riesco commented Apr 12, 2017 •

edited

Loading

etpinard May 10, 2017 •

edited

Loading