Array support for trace `hoverinfo` #1761

etpinard · 2017-06-05T17:49:06Z

resolves #1749

TODO:

implement for 3d, gl2d, pie - done in e8a9ddd

- make base hoverinfo `arrayOk: true` - merge array hoverinfo in calcdata while coercing its flags - look for per-point hoverinfo, fallback to trace hoverinfo - add checks string (vs array) hoverinfo values

- which is now handled during defaults, where one-trace graphs have no `'name'` hoverinfo flag by default - this fixes the set `hoverinfo: 'all'` + 1 trace graph hover labels, the name label is now properly displayed.

etpinard · 2017-06-05T18:40:58Z

src/components/fx/calc.js

+        }
+
+        return function(val) {
+            return Lib.coerce({hoverinfo: val}, {}, attrs, 'hoverinfo', dflt);


This here coerces hoverinfo items with no valid flags to 'all' if gd.data.length > 1 or all flags but 'name' on single-trace graphs. For example,

hoveinfo: ['sda', null, 'hello+sup'] // assuming more than one trace, // => gets coerced to hoverinfo: ['all', 'all', 'all']

This is debatable I feel. Perhaps, hoverinfo items with no valid flags should get coerced to 'none' instead? This would be more in line with e.g. marker.color where

marker.color: ['red', null, 'blue'] // gets coerced to marker.color: ['red', '#444' /* i.e. Color.defaultLine */, 'blue']

I'm currently leaning toward the second option, but I have no strong opinion.

I think as you have it is probably correct - an invalid entry gets the default behavior, which is 'all' or all but name. This could come in handy, in as far as the default is really what you want most of the time, for tweaking hoverinfo of just a few points by providing those in the array and leaving the rest of the array undefined or null.

- there's no 'c' array in scattercarpet

- which handles the 1-vs-multi-trace dflt logic for all trace types - call it once (!) during defaults - call it again during fx/calc to coerce arrayOk hoverinfo items

- pie trace array hoverlabel settings were broken before this patch when pie `trace.sort = true` (the default by the way) as this reorders the calcdata items which led to fx/calc filling hoverlabel field in the wrong calcdata items. - to solve the problem, simply don't rely on calcdata for hoverlabel setting in pie traces (as we currently do for gl3d and gl2d).

- To do so, add Fx.castHoverinfo helper and use it during gl3d, gl2d and pie hover where hoverinfo (trace-wide and per-pt array) is extracted from fullData

etpinard · 2017-06-06T01:35:41Z

src/traces/pie/plot.js

-                        fontFamily: pt.htf || hoverLabelOpts.font.family,
-                        fontSize: pt.hts || hoverLabelOpts.font.size,
-                        fontColor: pt.htc || hoverLabelOpts.font.color
+                        color: Fx.castHoverOption(trace, pt.i, 'bgcolor') || pt.color,


I never realized that pie/calc.js sorts calcdata items meaning the fullData array order isn't preserved. That is, we can't fill in pie calcdata items with hoverlabel field in order. Luckily here, we can simply reuse the gl2d/gl3d machinery.

This commit fixes pie traces with array hoverlabel values.

This commit fixes pie traces with array hoverlabel values.

Good catch! Is this covered in the tests? If it is, it's not obvious to me which one...

Is this covered in the tests?

yes

... which was previously broken.

Great - I saw that change, just couldn't tell why it tested this behavior - but a failure is a failure, works for me!

etpinard · 2017-06-06T01:38:24Z

src/lib/coerce.js

+ * @param {object} layoutOut : full layout object (require _dataLength ref)
+ * @return {any} : the coerced value
+ */
+exports.coerceHoverinfo = function(traceIn, traceOut, layoutOut) {


I hope nobody minds this abstraction. The hoverinfo dflt is ✨ : special ✨ , so I think it deserves its own coerce function.

etpinard · 2017-06-06T01:38:41Z

src/traces/scattercarpet/attributes.js

@@ -117,7 +117,7 @@ module.exports = {
    textfont: scatterAttrs.textfont,
    textposition: scatterAttrs.textposition,
    hoverinfo: extendFlat({}, plotAttrs.hoverinfo, {
-        flags: ['a', 'b', 'c', 'text', 'name']
+        flags: ['a', 'b', 'text', 'name']


fyi @rreusser

- e.g. hoverinfo, hoverlabel.bgcolor etc ...

etpinard · 2017-06-06T15:14:23Z

test/jasmine/tests/transform_filter_test.js

+
+        expect(out[0].x).toEqual([1, 2, 3]);
+        expect(out[0].y).toEqual([2, 3, 1]);
+        expect(out[0].hoverinfo).toEqual(['none', 'skip', 'all']);


Before this patch, we had

out[0].hoverinfo // => ['x', 'y', 'text', 'name', 'none', 'skip', 'all']

alexcjohnson · 2017-06-06T15:30:08Z

src/plot_api/plot_schema.js

@@ -156,6 +156,7 @@ exports.findArrayAttributes = function(trace) {
        return stack.join('.');
    }

+    exports.crawl(baseAttributes, callback);


ha nice, that's a lot simpler than I was afraid it might be!

- previously fx attribute arrays with length less than the calcdata trace length were skipped. That is, they did not get filled with their default - this patch here ensures that the defaults values are present in the calcdata so that we don't have to rely on fallbacks in the hover hot code path.

etpinard · 2017-06-06T15:42:03Z

test/jasmine/tests/hover_label_test.js

+        })
+        .then(function() {
+            expect(gd.calcdata[0].map(function(o) { return o.hi; })).toEqual(
+                ['none', 'x+y+z+text', 'x+y+z+text'],


Before this commit, we had

gd.calcdata[0].map(o => o.hi) // => // ['none']

and no fallbacks for hoverinfo in the hover routine. Since, the hoverinfo default value isn't trivial, I chose add a (another 😑 ) Lib function fillArray.

Lib.fillArray is very similar to Lib.mergeArray, but instead of looping over the min of cd[i].length and hoverinfo.length, it loops over cd[i].length allowing calcdata items to be filled in with defaults.

alexcjohnson · 2017-06-06T15:53:23Z

src/lib/index.js

@@ -350,19 +350,25 @@ lib.noneOrAll = function(containerIn, containerOut, attrList) {
    }
 };

-/** merge data array into calcdata items
+lib.mergeArray = function(traceAttr, cd, cdAttr) {


🎎 with a description of the difference between mergeArray and fillArray? And perhaps some indication of why one would use merge vs fill? Makes me wonder whether any of our other existing merge usages should be fill...

mergeArray doesn't need fn?

🎎 with a description of the difference between mergeArray and fillArray?

Sure. 👍

Makes me wonder whether any of our other existing merge usages should be fill...

Good point. Many routines in Drawing have fallbacks in case a required calcdata field is missing. The fallback pattern works well when the fallback value is constant for all cases (e.g. missing items in a marker.color array get drawn as #444), but not so well when it is depends on other things (e.g. for hoverinfo here which depends on gd.data.length and trace.type).

That said, replacing all Lib.mergeArray calls with Lib.fillArray should not break anything. I could do that if you prefer. It might slow things down a little bit when e.g.

trace.x.length // => 1e5 trace.marker.color.length // => 2

but I'd call that an edge case.

mergeArray doesn't need fn?

Not at the moment.

That said, replacing all Lib.mergeArray calls with Lib.fillArray should not break anything. I could do that if you prefer. It might slow things down a little bit when e.g.

I'm not worried about perf - yes, shorter attribute array is an edge case I'm comfortable neglecting. But it does change behavior, even if it's behavior we expect users not to be depending on. I'd leave it as is for the moment, for existing uses, and make an issue about it. We'll probably have to look at each attribute individually to see if we think the change is sufficiently unambiguous as an improvement that it's worth the minor incompatibility.

🎎 done in 3c3fda4

alexcjohnson · 2017-06-06T15:57:24Z

src/lib/index.js

 *
 * @param {array} traceAttr : trace attribute
 * @param {object} cd : calcdata trace
 * @param {string} cdAttr : calcdata key
 * @param {function} [fn] : optional function to apply to each array item
 */
-lib.mergeArray = function(traceAttr, cd, cdAttr, fn) {
+lib.fillArray = function(traceAttr, cd, cdAttr, fn) {
    fn = fn || lib.identity;


🐎 I wonder whether it would be worth avoiding fn entirely if it's not provided? Either
cd[i][cdAttr] = fn ? fn(traceAttr[i]) : traceAttr[i];
or even

if(fn) { for(i = 0; i < cd.length; i++) cd[i][cdAttr] = fn(traceAttr[i]); } else { for(i = 0; i < cd.length; i++) cd[i][cdAttr] = traceAttr[i]; }

That won't make that much of a difference (at least in Chrome 58)

https://gist.github.com/etpinard/4a5c7983a619efb4c98cb927529b9af6

awesome, thanks for checking.

etpinard · 2017-06-06T16:51:14Z

tagging this as reviewable

alexcjohnson · 2017-06-06T16:57:02Z

No more comments from me - looks great! 💃

... 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 added 2 commits June 5, 2017 13:26

implement arrayOk hoverinfo in Fx.hover

aab3366

- make base hoverinfo `arrayOk: true` - merge array hoverinfo in calcdata while coercing its flags - look for per-point hoverinfo, fallback to trace hoverinfo - add checks string (vs array) hoverinfo values

rm obsolete gd.data.length -> pointData.name logic in Fx.hover

2acf548

- which is now handled during defaults, where one-trace graphs have no `'name'` hoverinfo flag by default - this fixes the set `hoverinfo: 'all'` + 1 trace graph hover labels, the name label is now properly displayed.

etpinard added status: in progress feature something new labels Jun 5, 2017

etpinard added this to the 1.28.0 milestone Jun 5, 2017

etpinard commented Jun 5, 2017

View reviewed changes

etpinard added 4 commits June 5, 2017 21:23

fix scattercarpet hoverinfo flags declaration

b649b11

- there's no 'c' array in scattercarpet

add Lib.coerceHoverinfo shortcut

7ed8681

- which handles the 1-vs-multi-trace dflt logic for all trace types - call it once (!) during defaults - call it again during fx/calc to coerce arrayOk hoverinfo items

add arrayOk support for hoverinfo for gl3d, gl2d and pie traces

e8a9ddd

- To do so, add Fx.castHoverinfo helper and use it during gl3d, gl2d and pie hover where hoverinfo (trace-wide and per-pt array) is extracted from fullData

etpinard commented Jun 6, 2017

View reviewed changes

make findArrayAttributes track arrayOk attrs in base trace attrs

3c693a0

- e.g. hoverinfo, hoverlabel.bgcolor etc ...

etpinard commented Jun 6, 2017

View reviewed changes

alexcjohnson reviewed Jun 6, 2017

View reviewed changes

etpinard commented Jun 6, 2017

View reviewed changes

alexcjohnson reviewed Jun 6, 2017

View reviewed changes

etpinard added status: reviewable and removed status: in progress labels Jun 6, 2017

🎎 for Lib.mergeArray vs Lib.fillArray

3c3fda4

fix typo in 🎎

d1f03c0

etpinard merged commit 9779c96 into master Jun 6, 2017

etpinard deleted the arrayok-hoverinfo branch June 6, 2017 17:09

etpinard mentioned this pull request Jun 27, 2017

Hover label namelength #1822

Merged

etpinard mentioned this pull request Sep 29, 2017

scattergeo hoverinfo breaks when it's an array #2044

Closed

etpinard mentioned this pull request Oct 3, 2017

ArrayOk hoverinfo fixups #2055

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Array support for trace `hoverinfo` #1761

Array support for trace `hoverinfo` #1761

etpinard commented Jun 5, 2017 •

edited

Loading

etpinard Jun 5, 2017

alexcjohnson Jun 5, 2017

etpinard Jun 6, 2017

alexcjohnson Jun 6, 2017

etpinard Jun 6, 2017

etpinard Jun 6, 2017

alexcjohnson Jun 6, 2017

etpinard Jun 6, 2017 •

edited

Loading

etpinard Jun 6, 2017

etpinard Jun 6, 2017

alexcjohnson Jun 6, 2017

etpinard Jun 6, 2017

alexcjohnson Jun 6, 2017

etpinard Jun 6, 2017

alexcjohnson Jun 6, 2017

etpinard Jun 6, 2017 •

edited

Loading

alexcjohnson Jun 6, 2017

etpinard Jun 6, 2017

alexcjohnson Jun 6, 2017

etpinard commented Jun 6, 2017

alexcjohnson commented Jun 6, 2017

Array support for trace hoverinfo #1761

Array support for trace hoverinfo #1761

Conversation

etpinard commented Jun 5, 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

etpinard Jun 6, 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

etpinard Jun 6, 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

etpinard commented Jun 6, 2017

alexcjohnson commented Jun 6, 2017

Array support for trace `hoverinfo` #1761

Array support for trace `hoverinfo` #1761

etpinard commented Jun 5, 2017 •

edited

Loading

etpinard Jun 6, 2017 •

edited

Loading

etpinard Jun 6, 2017 •

edited

Loading