8000 ArrayOk hoverinfo fixups by etpinard · Pull Request #2055 · plotly/plotly.js · GitHub
[go: up one dir, main page]
Skip to content

ArrayOk hoverinfo fixups #2055

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Oct 5, 2017
Merged

ArrayOk hoverinfo fixups #2055

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4cf489e
introduce assertHoverLabelContent custom assertion
etpinard Oct 3, 2017
90e2de1
replace hover-label test boiler-plate with assertHoverLabelContent
etpinard Oct 3, 2017
c89c895
move scattergeo & choropleth hover label test trace module suits
etpinard Oct 3, 2017
b24759d
fix scattercarpet 'y' hover format + add tests
etpinard Oct 3, 2017
0a77239
fixes #2044 - add hoverinfo arrayOk support to ...
etpinard Oct 3, 2017
85d7061
allow choropleth trace to have (trace-wide) string 'text' values
etpinard Oct 3, 2017
bc8294d
add Lib.extractOption
etpinard Oct 4, 2017
b637179
add fillHoverText routine
etpinard Oct 4, 2017
294714b
improve assertHoverLabelContent API
etpinard Oct 4, 2017
2fe641d
replace ' = ' for ': ' in scattercarpet hover label prefixes
etpinard Oct 4, 2017
7f2604a
use extractOption in choropleth hover
etpinard Oct 4, 2017
6a44a9a
fix panning on text chart with invalid array values
etpinard Oct 4, 2017
3931273
make extractOption less opinionated
etpinard Oct 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/components/drawing/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,12 +443,10 @@ drawing.tryColorscale = function(marker, prefix) {
var TEXTOFFSETSIGN = {start: 1, end: -1, middle: 0, bottom: 1, top: -1};
drawing.textPointStyle = function(s, trace, gd) {
s.each(function(d) {
var p = d3.select(this),
text = d.tx || trace.text;
var p = d3.select(this);
var text = Lib.extractOption(d, trace, 'tx', 'text');

if(!text || Array.isArray(text)) {
// isArray test handles the case of (intentionally) missing
// or empty text within a text array
if(!text) {
p.remove();
return;
}
Expand Down
26 changes: 12 additions & 14 deletions src/components/fx/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1028,24 +1028,22 @@ function alignHoverText(hoverLabels, rotateLabels) {
}

function cleanPoint(d, hovermode) {
var index = d.index;
var trace = d.trace || {};
var cd0 = d.cd[0];
var cd = d.cd[d.index] || {};
var cd = d.cd[index] || {};

var getVal = Array.isArray(index) ?
function(calcKey, traceKey) {
return Lib.castOption(cd0, index, calcKey) ||
Lib.extractOption({}, trace, '', traceKey);
} :
function(calcKey, traceKey) {
return Lib.extractOption(cd, trace, calcKey, traceKey);
};

function fill(key, calcKey, traceKey) {
var val;

if(cd[calcKey]) {
val = cd[calcKey];
} else if(cd0[calcKey]) {
var arr = cd0[calcKey];
if(Array.isArray(arr) && Array.isArray(arr[d.index[0]])) {
val = arr[d.index[0]][d.index[1]];
}
} else {
val = Lib.nestedProperty(trace, traceKey).get();
}

var val = getVal(calcKey, traceKey);
if(val) d[key] = val;
}

Expand Down
21 changes: 21 additions & 0 deletions src/lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,27 @@ lib.castOption = function(trace, ptNumber, astr, fn) {
}
};

/** Extract option from calcdata item, correctly falling back to
* trace value if not found.
*
* @param {object} calcPt : calcdata[i][j] item
* @param {object} trace : (full) trace object
* @param {string} calcKey : calcdata key
* @param {string} traceKey : aka trace attribute string
* @return {any}
*/
lib.extractOption = function(calcPt, trace, calcKey, traceKey) {
var calcVal = calcPt[calcKey];
if(calcVal || calcVal === 0) return calcVal;

// fallback to trace value,
// must check if value isn't itself an array
// 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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author
@etpinard etpinard Oct 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

image

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

So, I think both are acceptable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

};

/** Returns target as set by 'target' transform attribute
*
* @param {object} trace : full trace object
Expand Down
6 changes: 2 additions & 4 deletions src/traces/choropleth/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,9 @@ module.exports = extendFlat({
editType: 'calc',
description: 'Sets the color values.'
},
text: {
valType: 'data_array',
editType: 'calc',
text: extendFlat({}, ScatterGeoAttrs.text, {
description: 'Sets the text elements associated with each location.'
},
}),
marker: {
line: {
color: ScatterGeoMarkerLineAttrs.color,
Expand Down
21 changes: 14 additions & 7 deletions src/traces/choropleth/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,17 +53,17 @@ module.exports = function hoverPoints(pointData, xval, yval) {
};

function makeHoverInfo(pointData, trace, pt, axis) {
var hoverinfo = trace.hoverinfo;
var hoverinfo = pt.hi || trace.hoverinfo;

var parts = (hoverinfo === 'all') ?
attributes.hoverinfo.flags :
hoverinfo.split('+');

var hasName = (parts.indexOf('name') !== -1),
hasLocation = (parts.indexOf('location') !== -1),
hasZ = (parts.indexOf('z') !== -1),
hasText = (parts.indexOf('text') !== -1),
hasIdAsNameLabel = !hasName && hasLocation;
var hasName = (parts.indexOf('name') !== -1);
var hasLocation = (parts.indexOf('location') !== -1);
var hasZ = (parts.indexOf('z') !== -1);
var hasText = (parts.indexOf('text') !== -1);
var hasIdAsNameLabel = !hasName && hasLocation;

var text = [];

Expand All @@ -79,7 +79,14 @@ function makeHoverInfo(pointData, trace, pt, axis) {
}

if(hasZ) text.push(formatter(pt.z));
if(hasText) text.push(pt.tx);
if(hasText) {
var tx;

if(pt.tx) tx = pt.tx;
else if(trace.text) tx = trace.text;

if(!Array.isArray(tx)) text.push(tx);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See bc8294d and b637179 for what I came up with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

pointData.extraText = text.join('<br>');
}
17 changes: 9 additions & 8 deletions src/traces/scattercarpet/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,19 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
newPointData.yLabelVal = undefined;
// TODO: nice formatting, and label by axis title, for a, b, and c?

var trace = newPointData.trace,
carpet = trace._carpet,
hoverinfo = trace.hoverinfo.split('+'),
text = [];
var trace = newPointData.trace;
var carpet = trace._carpet;
var hoverinfo = cdi.hi || trace.hoverinfo;
var parts = hoverinfo.split('+');
var text = [];

function textPart(ax, val) {
text.push(((ax.labelprefix && ax.labelprefix.length > 0) ? ax.labelprefix : (ax._hovertitle + ': ')) + val.toFixed(3) + ax.labelsuffix);
}

if(hoverinfo.indexOf('all') !== -1) hoverinfo = ['a', 'b'];
if(hoverinfo.indexOf('a') !== -1) textPart(carpet.aaxis, cdi.a);
if(hoverinfo.indexOf('b') !== -1) textPart(carpet.baxis, cdi.b);
if(parts.indexOf('all') !== -1) parts = ['a', 'b'];
if(parts.indexOf('a') !== -1) textPart(carpet.aaxis, cdi.a);
if(parts.indexOf('b') !== -1) textPart(carpet.baxis, cdi.b);

var ij = carpet.ab2ij([cdi.a, cdi.b]);
var i0 = Math.floor(ij[0]);
Expand All @@ -67,7 +68,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));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

peek 2017-10-03 17-01

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Thank you. That seems correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 2fe641d


newPointData.extraText = text.join('<br>');

Expand Down
24 changes: 13 additions & 11 deletions src/traces/scattergeo/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,29 +71,31 @@ module.exports = function hoverPoints(pointData, xval, yval) {
};

function getExtraText(trace, pt, axis) {
var hoverinfo = trace.hoverinfo;
var hoverinfo = pt.hi || trace.hoverinfo;

var parts = (hoverinfo === 'all') ?
var parts = hoverinfo === 'all' ?
attributes.hoverinfo.flags :
hoverinfo.split('+');

var hasLocation = parts.indexOf('location') !== -1 && Array.isArray(trace.locations),
hasLon = (parts.indexOf('lon') !== -1),
hasLat = (parts.indexOf('lat') !== -1),
hasText = (parts.indexOf('text') !== -1);

var hasLocation = parts.indexOf('location') !== -1 && Array.isArray(trace.locations);
var hasLon = (parts.indexOf('lon') !== -1);
var hasLat = (parts.indexOf('lat') !== -1);
var hasText = (parts.indexOf('text') !== -1);
var text = [];

function format(val) {
return Axes.tickText(axis, axis.c2l(val), 'hover').text + '\u00B0';
}

if(hasLocation) text.push(pt.loc);
else if(hasLon && hasLat) {
if(hasLocation) {
text.push(pt.loc);
} else if(hasLon && hasLat) {
text.push('(' + format(pt.lonlat[0]) + ', ' + format(pt.lonlat[1]) + ')');
} else if(hasLon) {
text.push('lon: ' + format(pt.lonlat[0]));
} else if(hasLat) {
text.push('lat: ' + format(pt.lonlat[1]));
}
else if(hasLon) text.push('lon: ' + format(pt.lonlat[0]));
else if(hasLat) text.push('lat: ' + format(pt.lonlat[1]));

if(hasText) {
var tx;
Expand Down
21 changes: 12 additions & 9 deletions src/traces/scattermapbox/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,14 @@ module.exports = function hoverPoints(pointData, xval, yval) {
};

function getExtraText(trace, di) {
var hoverinfo = trace.hoverinfo.split('+'),
isAll = (hoverinfo.indexOf('all') !== -1),
hasLon = (hoverinfo.indexOf('lon') !== -1),
hasLat = (hoverinfo.indexOf('lat') !== -1);
var hoverinfo = di.hi || trace.hoverinfo;
var parts = hoverinfo.split('+');
var isAll = parts.indexOf('all') !== -1;
var hasLon = parts.indexOf('lon') !== -1;
var hasLat = parts.indexOf('lat') !== -1;

var lonlat = di.lonlat,
text = [];
var lonlat = di.lonlat;
var text = [];

// TODO should we use a mock axis to format hover?
// If so, we'll need to make precision be zoom-level dependent
Expand All @@ -82,11 +83,13 @@ function getExtraText(trace, di) {

if(isAll || (hasLon && hasLat)) {
text.push('(' + format(lonlat[0]) + ', ' + format(lonlat[1]) + ')');
} else if(hasLon) {
text.push('lon: ' + format(lonlat[0]));
} else if(hasLat) {
text.push('lat: ' + format(lonlat[1]));
}
else if(hasLon) text.push('lon: ' + format(lonlat[0]));
else if(hasLat) text.push('lat: ' + format(lonlat[1]));

if(isAll || hoverinfo.indexOf('text') !== -1) {
if(isAll || parts.indexOf('text') !== -1) {
var tx;

if(di.htx) tx = di.htx;
Expand Down
17 changes: 9 additions & 8 deletions src/traces/scatterternary/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,19 +49,20 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
newPointData.yLabelVal = undefined;
// TODO: nice formatting, and label by axis title, for a, b, and c?

var trace = newPointData.trace,
ternary = trace._ternary,
hoverinfo = trace.hoverinfo.split('+'),
text = [];
var trace = newPointData.trace;
var ternary = trace._ternary;
var hoverinfo = cdi.hi || trace.hoverinfo;
var parts = hoverinfo.split('+');
var text = [];

function textPart(ax, val) {
text.push(ax._hovertitle + ': ' + Axes.tickText(ax, val, 'hover').text);
}

if(hoverinfo.indexOf('all') !== -1) hoverinfo = ['a', 'b', 'c'];
if(hoverinfo.indexOf('a') !== -1) textPart(ternary.aaxis, cdi.a);
if(hoverinfo.indexOf('b') !== -1) textPart(ternary.baxis, cdi.b);
if(hoverinfo.indexOf('c') !== -1) textPart(ternary.caxis, cdi.c);
if(parts.indexOf('all') !== -1) parts = ['a', 'b', 'c'];
if(parts.indexOf('a') !== -1) textPart(ternary.aaxis, cdi.a);
if(parts.indexOf('b') !== -1) textPart(ternary.baxis, cdi.b);
if(parts.indexOf('c') !== -1) textPart(ternary.caxis, cdi.c);

newPointData.extraText = text.join('<br>');

Expand Down
86 changes: 86 additions & 0 deletions test/jasmine/assets/custom_assertions.js
View file
Open in desktop
6377 7F6F
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good eyes. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 294714b

var lineCnt = lines.size();
var expectMultiLine = Array.isArray(expectation);

function extract(sel) {
return sel.node() ? sel.html() : null;
}

if(lineCnt > 0) {
if(expectMultiLine) {
expect(lines.size()).toBe(expectation.length, msg + ': # of lines');
lines.each(function(_, i) {
var l = d3.select(this);
expect(extract(l)).toBe(expectation[i], msg + ': tspan line ' + i);
});
} else {
fail('Expected a single-line label, found multiple lines');
}
} else {
if(!expectMultiLine) {
expect(extract(label)).toBe(expectation, msg + ': text content');
} else {
fail('Expected a multi-line label, found single');
}
}
}

function count(selector) {
return d3.selectAll(selector).size();
}

exports.assertHoverLabelContent = function(expectation, msg) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 294714b

Thoughts?

if(!msg) msg = '';

var ptSelector = 'g.hovertext';
var ptExpectation = expectation[0];
var ptMsg = msg + ' point hover label';
var ptCnt = count(ptSelector);

var axSelector = 'g.axistext';
var axExpectation = expectation[1];
var axMsg = 'common axis hover label';
var axCnt = count(axSelector);

if(ptCnt === 1) {
assertLabelContent(
d3.select(ptSelector + '> text.nums'),
ptExpectation[0],
ptMsg + ' (nums)'
);
assertLabelContent(
d3.select(ptSelector + '> text.name'),
ptExpectation[1],
ptMsg + ' (name)'
);
} else if(ptCnt > 1) {
expect(ptCnt).toBe(ptExpectation.length, ptMsg + ' # of visible nodes');

d3.selectAll(ptSelector).each(function(_, i) {
assertLabelContent(
d3.select(this).select('text.nums'),
ptExpectation[i][0],
ptMsg + ' (nums ' + i + ')'
);
assertLabelContent(
d3.select(this).select('text.name'),
ptExpectation[i][1],
ptMsg + ' (name ' + i + ')'
);
});
} else {
expect(ptExpectation).toBe(null, ptMsg + ' should not be displayed');
}

if(axCnt > 0) {
assertLabelContent(
d3.select(axSelector + '> text'),
axExpectation,
axMsg
);
} else {
expect(axExpectation).toBe(null, axMsg + ' should not be displayed');
}
};

exports.assertClip = function(sel, isClipped, size, msg) {
expect(sel.size()).toBe(size, msg + ' clip path (selection size)');

Expand Down
Loading
0