ArrayOk hoverinfo fixups #2055

etpinard · 2017-10-03T21:00:33Z

fixes #2044 in 0a77239, but really, these few commits here should have been part of #1761. Adding anything hover-related is still a bit of a pain right now. As a lot of logic is scattered around components/fx, the base plot and trace modules, things don't just work unfortunately. Manually checking every trace type is in order until we better centralize our hover label logic.

This PR also add a fairly general custom assertion in 4cf489e that checks every part of the hover label DOM nodes. I also caught a few others bug fixed and 🔒 in b24759d and 85d7061.

cc @alexcjohnson

... scattergeo, choropleth, scatterternary and scattermapbox - these are all the trace type that rely on `extraText` built in their _module.hoverPoints method - this commit should've been part of PR #1761

etpinard · 2017-10-03T21:02:19Z

src/traces/scattercarpet/hover.js

@@ -67,7 +67,7 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
    var tj = ij[1] - j0;

    var xy = carpet.evalxy([], i0, j0, ti, tj);
-    text.push('y: ' + xy[1].toFixed(3));
+    text.push('y = ' + xy[1].toFixed(3));


cc @rreusser unless that y: was on purpose? This here turns it into y = just like the a = and b = rows

Most other places (heatmap/contour, 3d, ternary) we use : - perhaps we should standardize on that here too?

Good catch. Thank you. That seems correct.

Oh. Haha, my mistake then. Perhaps it's the a/b = that are the odd ones out. Need to check the consistent choice here.

https://github.com/plotly/plotly.js/blob/master/src/components/fx/hover.js#L697-L698

Sorry, to be less vague, : seems correct. = seems incorrect.

done in 2fe641d

alexcjohnson · 2017-10-03T21:48:48Z

src/traces/choropleth/hover.js

+        if(pt.tx) tx = pt.tx;
+        else if(trace.text) tx = trace.text;
+
+        if(!Array.isArray(tx)) text.push(tx);


nice careful logic - tx could be an array if trace.text is too short to populate pt.tx or if it has falsey elements (hmm, what about 0?)

This seems like a bit of logic that's repeated a bunch - and is probably done wrong in some places where it's not repeated... worth abstracting?

Worth it for sure. Shouldn't be too bad.

(the hardest part will be to figure out how to call this new function)

See bc8294d and b637179 for what I came up with.

Ooops I forget to update choropleth/hover.js, now done in -> 7f2604a

alexcjohnson · 2017-10-03T22:03:43Z

test/jasmine/assets/custom_assertions.js

@@ -59,6 +59,92 @@ exports.assertHoverLabelStyle = function(g, expectation, msg, textSelector) {
    expect(textStyle.fill).toBe(expectation.fontColor, msg + ': font.color');
 };

+function assertLabelContent(label, expectation, msg) {
+    var lines = label.selectAll('tspan');


in case we want to test styled labels at some point, selectAll('tspan.line')

Good eyes. Thanks!

done in 294714b

alexcjohnson · 2017-10-03T22:08:39Z

test/jasmine/assets/custom_assertions.js

+    return d3.selectAll(selector).size();
+}
+
+exports.assertHoverLabelContent = function(expectation, msg) {


I like this fn - really nice idea! I find the signature a little unintuitive though... expectation has to be something like [['a = 0.200', 'b = 3.500', 'y = 2.900'], 'a = 0.2']... perhaps it would be clearer like {nums: 'a = 0.200\nb = 3.500\ny = 2.900', name: 'a = 0.2'}? Array vs multiline string I don't feel that strongly about, but I think named items instead of the outer array would be quite a bit easier to read, and possibly easier to write too.

I get annoyed of typing object keys in assertion functions arguments when writing tests.

But I guess there's value in being a little more verbose for project-wide custom assertions.

... and good call about the somewhat useless single-vs-multi line logic. Our convertToTspans tests are there for that.

done in 294714b

Thoughts?

- abstracting out calcdata-to-global-trace-fallback - use it in Drawing! - use it in hover label style logic - not super happy with the name - not super happy about it being on Lib

- which handles calc/trace-wide 'hovertext'/'text' precedence logic - use it in hover.js modules

alexcjohnson · 2017-10-04T20:16:06Z

src/lib/index.js

+    //   which means the trace attribute has a corresponding
+    //   calcdata key, but its value is falsy
+    var traceVal = lib.nestedProperty(trace, traceKey).get();
+    if(!Array.isArray(traceVal)) return traceVal;


Nice, I like it. Thanks for accepting 0.

not super happy with the name

meh, it's a very specific concept that we probably don't want to go into any more detail with in the name. Seems fine to me.

not super happy about it being on Lib

It definitely belongs next to castOption but yeah, if we make a common/ that might be more appropriate for these.

Also a style question: I just noticed all over this file we start the docs immediately after /**, I thought the convention was to leave that first line blank and start on the next line?

Also a style question: I just noticed all over this file we start the docs immediately after /**, I thought the convention was to leave that first line blank and start on the next line?

from http://usejsdoc.org/howto-commonjs-modules.html

So, I think both are acceptable.

perhaps overly pedantic, but I read that as /** Single-line description */ or

/** * Multiline still looks like it always * leaves the first line blank. */

alexcjohnson · 2017-10-04T20:19:28Z

src/traces/scatter/fill_hover_text.js

+
+    var tx = Lib.extractOption(calcPt, trace, 'tx', 'text');
+    if(tx && tx !== '') return fill(tx);
+};


Nice! 🌴 Does this really belong in scatter though? Perhaps in fx?

I have no strong opinion here. This routine felt similar to

https://github.com/plotly/plotly.js/blob/master/src/traces/scatter/arrays_to_calcdata.js

that's why I put it in traces/scatter/

That's fine, we certainly have a long history of traces pulling from each other, particularly from scatter, and there is something nice about it being in the traces dir. Perhaps at some point we introduce a traces/common dir for stuff like this but leave it as is for now.

alexcjohnson · 2017-10-04T20:30:33Z

test/jasmine/assets/custom_assertions.js

+ *  - nums {string || array of strings}
+ *  - name {string || array of strings}
+ *  - axis {string}
+ * @param {string} msg


Thanks - I find that a lot easier, especially as you point out since this lives in a different file from where it gets called.

alexcjohnson · 2017-10-04T21:11:42Z

src/traces/choropleth/hover.js

-
-        if(!Array.isArray(tx)) text.push(tx);
+        var tx = Lib.extractOption(pt, trace, 'tx', 'text');
+        if(tx && tx !== '') text.push(tx);


'' is already falsey, right?

Good catch. Strictly speaking, I think this should be:

if(tx !== undefined && tx !== '') { text.push(tx); }

as Lib.extractOption returns undefined for all falsy values except 0 in calcdata. But, trace.text is allowed to be '' (which is the default by the way).

To make this easier to maintain (and remember 🙂 ), I'm thinking either

(1) we could remove the dflt: '' line in the text (and hovertext) attribute declarations. This leads to a slight change in behavior (but might more consistent) as now:

{ text: 'on-chart text', hovertext: '' }

would use '' for the hover label text field (i.e. not show anything). On master, hovertext: '' is ignored and 'on-chart text' shows in the hover labels.

(2) we could make Lib.extractOption return undefined when trace.text: '', making it a little more opinionated (I think this only makes sense for string attributes).

Here's what I came up with 3931273

I made Lib.extractOption less opinionated, and made scatter/fill_hover_text handle falsy but valid text values.

Yep, I like it. There are some edge cases this makes a little weird - eg trying to make one point have text but no hovertext, while all the others have both - but there are ways to get what you want (in that example using hoverinfo as an array) and I don't see any way to make everything more intuitive. So lets keep this, it's nice clean logic, will make sense to people, and will do what people expect 99.9% of the time.

alexcjohnson · 2017-10-04T21:28:38Z

Take a look at 7f2604a#r142793932 but other than that I'm happy - patching a functionality hole while DRYing up a bunch of things -> 💃 !

etpinard · 2017-10-04T22:24:39Z

src/components/drawing/index.js

@@ -887,6 +887,9 @@ drawing.setTextPointsScale = function(selection, xScale, yScale) {
        var transforms;
        var el = d3.select(this);
        var text = el.select('text');
+
+        if(!text.node()) return;


Caught another bug. dragmode: 'pan' is broken on http://rickyreusser.com/plotly-mock-viewer/#text_chart_arrays

This fixes it:

ha, good catch! 🎉

- by returning falsy calcdata values - use fillHoverText in choropleth/hover, for consistency

etpinard added 6 commits October 3, 2017 16:15

introduce assertHoverLabelContent custom assertion

4cf489e

replace hover-label test boiler-plate with assertHoverLabelContent

90e2de1

move scattergeo & choropleth hover label test trace module suits

c89c895

fix scattercarpet 'y' hover format + add tests

b24759d

fixes #2044 - add hoverinfo arrayOk support to ...

0a77239

... scattergeo, choropleth, scatterternary and scattermapbox - these are all the trace type that rely on `extraText` built in their _module.hoverPoints method - this commit should've been part of PR #1761

allow choropleth trace to have (trace-wide) string 'text' values

85d7061

etpinard added status: reviewable bug something broken labels Oct 3, 2017

etpinard commented Oct 3, 2017

View reviewed changes

alexcjohnson reviewed Oct 3, 2017

View reviewed changes

etpinard added 4 commits October 4, 2017 14:22

add Lib.extractOption

bc8294d

- abstracting out calcdata-to-global-trace-fallback - use it in Drawing! - use it in hover label style logic - not super happy with the name - not super happy about it being on Lib

add fillHoverText routine

b637179

- which handles calc/trace-wide 'hovertext'/'text' precedence logic - use it in hover.js modules

improve assertHoverLabelContent API

294714b

replace ' = ' for ': ' in scattercarpet hover label prefixes

2fe641d

alexcjohnson reviewed Oct 4, 2017

View reviewed changes

use extractOption in choropleth hover

7f2604a

alexcjohnson reviewed Oct 4, 2017

View reviewed changes

fix panning on text chart with invalid array values

6a44a9a

etpinard commented Oct 4, 2017

View reviewed changes

make extractOption less opinionated

3931273

- by returning falsy calcdata values - use fillHoverText in choropleth/hover, for consistency

etpinard merged commit c09d2e4 into master Oct 5, 2017

etpinard deleted the arrayok-hoverinfo-fixups branch October 5, 2017 15:24

alexcjohnson added a commit that referenced this pull request Oct 5, 2017

tweak purge related to @etpinard's extra fix in #2055

8d3ef29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ArrayOk hoverinfo fixups #2055

ArrayOk hoverinfo fixups #2055

etpinard commented Oct 3, 2017

etpinard Oct 3, 2017

alexcjohnson Oct 3, 2017

rreusser Oct 3, 2017

rreusser Oct 3, 2017

rreusser Oct 3, 2017

rreusser Oct 3, 2017

etpinard Oct 4, 2017

alexcjohnson Oct 3, 2017

etpinard Oct 3, 2017

etpinard Oct 3, 2017

etpinard Oct 4, 2017

etpinard Oct 4, 2017

alexcjohnson Oct 3, 2017

etpinard Oct 3, 2017

etpinard Oct 4, 2017

alexcjohnson Oct 3, 2017

etpinard Oct 3, 2017

etpinard Oct 3, 2017

etpinard Oct 4, 2017

alexcjohnson Oct 4, 2017

etpinard Oct 4, 2017 •

edited

Loading

alexcjohnson Oct 4, 2017

alexcjohnson Oct 4, 2017

etpinard Oct 4, 2017

alexcjohnson Oct 4, 2017

alexcjohnson Oct 4, 2017

alexcjohnson Oct 4, 2017

etpinard Oct 4, 2017

etpinard Oct 5, 2017

alexcjohnson Oct 5, 2017

alexcjohnson commented Oct 4, 2017

etpinard Oct 4, 2017

alexcjohnson Oct 5, 2017

ArrayOk hoverinfo fixups #2055

ArrayOk hoverinfo fixups #2055

Conversation

etpinard commented Oct 3, 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

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 Oct 4, 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 Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Oct 4, 2017 •

edited

Loading