Add hoverlabel.zformat attribute #1

jdugge · 2017-10-15T19:26:17Z

This PR adds the hoverlabel.zformat attribute, which controls the formatting of the z-values in hover labels.
The attribute can be set both at the trace and layout level.
If hoverlabel.zformat is missing or set to undefined, no formatting will occur, leaving the current behaviour unchanged.

etpinard

Thanks for the PR!

etpinard · 2017-10-16T14:06:29Z

src/components/fx/attributes.js

@@ -50,6 +50,17 @@ module.exports = {
                '`namelength - 3` characters and add an ellipsis.'
            ].join(' ')
        },
+        zformat: {


We can't declare zformat in fx/ as it only makes sense for traces with z data array (e.g. heatmap, contour and surface).

... we wouldn't want traces w/o a z attribute to have a zformat under their name on https://plot.ly/javascript/reference/

... this has me thinking - maybe my hoverlabel.zformat idea of plotly#1383 (comment) was wrong. It might be best to keep the hoverlabel attribute container as is that way that set of attributes will be the same for all trace types.

So maybe zhoverformat is the best option. I hate that it doesn't extend to other trace types very well. But I prefer this over breaking hoverlabel symmetry.

Any thoughts on this @alexcjohnson @cldougl ❓

So maybe zhoverformat is the best option. I hate that it doesn't extend to other trace types very well. But I prefer this over breaking hoverlabel symmetry.

I suppose the only other solution would be to move hoverlabel into each trace type attributes so it can be different for each one. But that's both a much more invasive change, and you're right there's something nice about keeping that container consistent in structure (though a lot of other containers, like marker and line, DO change from type to type... so it's not without precedent)

Whatever solution we adopt we should think about how it will extend to intensity and surfacecolor
plotly#1383 (comment) - surfacecolorhoverformat is getting a bit unwieldy but... so is hoverlabel.surfacecolorformat

So yeah, I guess zhoverformat is sounding better and better 😅

+1 for zhoverformat and keeping hoverlabel symmetry at this point.

etpinard · 2017-10-16T14:10:25Z

src/components/fx/hover.js

-    if(d.zLabelVal !== undefined) d.zLabel = String(d.zLabelVal);
+    if(d.zLabelVal !== undefined) {
+        if(d.zformat !== undefined) {
+            d.zLabel = d3.format(d.zformat)(d.zLabelVal).replace(/-/g, constants.MINUS_SIGN);


Please reuse Axes.tickText

Referencing plotly#1965, maybe hover-only format attributes could support the notion of stops as well.

The problem with using Axes.tickText is that heatmaps don't have an appropriate z-axis that could be passed to Axes.tickText. Should I create a dummy z-axis and use that, similar to what is being done here?

Should I create a dummy z-axis and use that, similar to what is being done here?

Yes, that's the approach we've used in several places. Actually I think we should do that anyway, even without a custom format, as basing the formatting on the actual range of z values in the data (ie formatting the hover text the default way we would on an x or y axis of the same range) would probably solve this issue in most cases anyway, without asking users to pick their own format.

etpinard · 2017-10-16T14:17:16Z

test/image/mocks/heatmap_hoverlabel.json

@@ -0,0 +1,39 @@
+{
+  "data": [


A new mock is ok, but we should really test the new behavior under

https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/heatmap_test.js#L592

https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/hover_label_test.js#L1238

etpinard · 2017-10-16T14:18:58Z

src/components/fx/hover.js

+        if(d.zformat !== undefined) {
+            d.zLabel = d3.format(d.zformat)(d.zLabelVal).replace(/-/g, constants.MINUS_SIGN);
+        } else {
+            d.zLabel = String(d.zLabelVal);


I would prefer adding the new formatting logic in the trace hoverPoints modules - e.g. this one.

jdugge · 2017-10-16T15:18:15Z

Thanks for the rapid and thorough feedback!

…ite tests

jdugge · 2017-10-18T20:39:03Z

Ready for another review. I've moved the option out of the hoverlabels and into the heatmap trace options and used the Axis.tickText method for formatting the labels.

etpinard

Looking good. This is almost ready for primetime!

To get this thing merged, you should at the very least add zhoverformat to contour traces. surface would benefit from zhoverformat too, but I can live without it for now (surface should get a surfacecolorformat attribute too!).

etpinard · 2017-10-18T22:36:06Z

src/components/fx/hover.js

@@ -1095,7 +1095,9 @@ function cleanPoint(d, hovermode) {
        d.yVal = d.ya.c2d(d.yLabelVal);
    }

-    if(d.zLabelVal !== undefined) d.zLabel = String(d.zLabelVal);
+    if(d.zLabelVal !== undefined && d.zLabel === undefined) { // Traces like heatmaps generate the zLabel in their hoverPoints function


Hmm. Too bad we need this. Good catch ⚾ cleanPoint could use a little bit of a refactor.

Would you mind putting that // comment about the if() statement?

etpinard · 2017-10-18T22:38:17Z

src/traces/heatmap/hover.js

@@ -99,6 +104,16 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode, contour)
        text = cd0.text[ny][nx];
    }

+    var zLabel;
+    var dummyAx = { // dummy axis for formatting the z value


Nicely done.

Creating the dummy axis here isn't optimal, but shouldn't make much of a difference. 👍

etpinard · 2017-10-18T22:41:49Z

src/traces/heatmap/hover.js

@@ -26,6 +27,10 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode, contour)
        y = cd0.y,
        z = cd0.z,
        zmask = cd0.zmask,
+        zmin = trace.zmin,


Don't scope variables if you're only going to use them once. The block below could be rewritten as:

var dummyAx = { type: 'linear', range: [trace.zmin, trace.zmax], hoverformat: trace.zhoverformat, _separators: trace._separators }; var zLabel = Axes.tickText(dummyAx, zVal, 'hover').text;

etpinard · 2017-10-18T22:42:57Z

test/jasmine/tests/hover_label_test.js

@@ -499,6 +499,43 @@ describe('hover info', function() {
            .catch(fail)
            .then(done);
        });
+
+        it('should display correct label content with specified format', function(done) {


Looks like the two test cases you added are pretty much equivalent. Pick your favorite and 🔪 the other.

jdugge · 2017-10-19T20:51:55Z

I added it to contour traces and removed one of the test cases.
I tried adding it to surface plots, but the z-hoverformat there can already be set using scene.zaxis.hoverformat, and it seems that the surfacecolor values are never shown on hovering so a surfacecolorhoverformat wouldn't do anything?

etpinard · 2017-10-19T21:05:53Z

but the z-hoverformat there can already be set using scene.zaxis.hoverformat, and it seems that the surfacecolor values are never shown on hovering so a surfacecolorhoverformat wouldn't do anything?

Oh good point. I forgot about that.

All right, looks like your PR is ready for the main repo. PR away 🚀

…bel.zformat

- PR plotly#2099 got merged on a branch behind plotly#2081 which caused the test to fail on master.

Fixup choropleth select test

into hoverlabel.zformat

Add hoverlabel.zformat attribute

acce9bf

jdugge mentioned this pull request Oct 15, 2017

Allow setting 'hoverformat' for z-values in heatmap plotly/plotly.js#1383

Closed

etpinard reviewed Oct 16, 2017

View reviewed changes

jdugge added 2 commits October 18, 2017 22:31

Move property out of hoverlabel, use Axis.tickText for formatting, wr…

b918176

…ite tests

Remove zformat from hoverlabels layout properties

7a27542

etpinard reviewed Oct 18, 2017

View reviewed changes

Add zhoverformat to contour plots

919feea

jdugge and others added 8 commits October 19, 2017 23:11

Add zhoverformat to heatmap and contour traces

5e20a48

Merge remote-tracking branch 'origin/hoverlabel.zformat' into hoverla…

a24be4f

…bel.zformat

Merge branch 'master' into hoverlabel.zformat

11f5edc

Fix formatting

86277f2

fixup choropleth select test

1760d66

- PR plotly#2099 got merged on a branch behind plotly#2081 which caused the test to fail on master.

Merge pull request plotly#2109 from plotly/fixup-choropleth-select-test

f991039

Fixup choropleth select test

Add zhoverformat to heatmap and contour traces

48509f3

Merge branch 'hoverlabel.zformat' of https://github.com/jdugge/plotly.js

5c0b2f7

into hoverlabel.zformat

jdugge closed this Oct 26, 2017

jdugge deleted the hoverlabel.zformat branch October 26, 2017 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hoverlabel.zformat attribute #1

Add hoverlabel.zformat attribute #1

jdugge commented Oct 15, 2017

etpinard left a comment

etpinard Oct 16, 2017

etpinard Oct 16, 2017

etpinard Oct 16, 2017

etpinard Oct 16, 2017

alexcjohnson Oct 16, 2017

cldougl Oct 16, 2017 •

edited

Loading

etpinard Oct 16, 2017

etpinard Oct 16, 2017

jdugge Oct 16, 2017

alexcjohnson Oct 16, 2017

etpinard Oct 16, 2017

etpinard Oct 16, 2017

jdugge commented Oct 16, 2017

jdugge commented Oct 18, 2017

etpinard left a comment

etpinard Oct 18, 2017

etpinard Oct 18, 2017

etpinard Oct 18, 2017

etpinard Oct 18, 2017

jdugge commented Oct 19, 2017 •

edited

Loading

etpinard commented Oct 19, 2017

Add hoverlabel.zformat attribute #1

Add hoverlabel.zformat attribute #1

Conversation

jdugge commented Oct 15, 2017

etpinard left a 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

cldougl Oct 16, 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

jdugge commented Oct 16, 2017

jdugge commented Oct 18, 2017

etpinard left a 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

jdugge commented Oct 19, 2017 • edited Loading

etpinard commented Oct 19, 2017

cldougl Oct 16, 2017 •

edited

Loading

jdugge commented Oct 19, 2017 •

edited

Loading