-
-
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 4 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 |
---|---|---|
|
@@ -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.
cc @rreusser unless that
y:
was on purpose? This here turns it intoy =
just like thea =
andb =
rowsThere 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.
Most other places (heatmap/contour, 3d, ternary) we use
:
- perhaps we should standardize on that here too?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.
Good catch. Thank you. That seems correct.
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.
Oh. Haha, my mistake then. Perhaps it's the
a/b =
that are the odd ones out. Need to check the consistent choice here.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.
https://github.com/plotly/plotly.js/blob/master/src/components/fx/hover.js#L697-L698
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.
Sorry, to be less vague,
:
seems correct.=
seems incorrect.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.
done in 2fe641d