-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
ArrayOk hoverinfo fixups #2055
ArrayOk hoverinfo fixups #2055
Changes from 6 commits
4cf489e
90e2de1
c89c895
b24759d
0a77239
85d7061
bc8294d
b637179
294714b
2fe641d
7f2604a
6a44a9a
3931273
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]); | ||
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @rreusser unless that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most other places (heatmap/contour, 3d, ternary) we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. Thank you. That seems correct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh. Haha, my mistake then. Perhaps it's the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, to be less vague, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done in 2fe641d |
||
|
||
newPointData.extraText = text.join('<br>'); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in case we want to test styled labels at some point, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good eyes. Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)'); | ||
|
||
|
There was a problem hiding this comment.
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 iftrace.text
is too short to populatept.tx
or if it has falsey elements (hmm, what about0
?)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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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