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 1 commit
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
Prev Previous commit
Next Next commit
improve assertHoverLabelContent API
  • Loading branch information
@etpinard
etpinard committed Oct 4, 2017
commit 294714b7585bf585f115285577ebdb0f21290e80
73 changes: 42 additions & 31 deletions test/jasmine/assets/custom_assertions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,88 +60,99 @@ exports.assertHoverLabelStyle = function(g, expectation, msg, textSelector) {
};

function assertLabelContent(label, expectation, msg) {
var lines = label.selectAll('tspan');
var lineCnt = lines.size();
var expectMultiLine = Array.isArray(expectation);
if(!expectation) expectation = '';

function extract(sel) {
return sel.node() ? sel.html() : null;
}
var lines = label.selectAll('tspan.line');
var content = [];

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');
function fill(sel) {
if(sel.node()) {
var html = sel.html();
if(html) content.push(html);
}
}

if(lines.size()) {
lines.each(function() { fill(d3.select(this)); });
} else {
if(!expectMultiLine) {
expect(extract(label)).toBe(expectation, msg + ': text content');
} else {
fail('Expected a multi-line label, found single');
}
fill(label);
}

expect(content.join('\n')).toBe(expectation, msg + ': text content');
}

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

/**
* @param {object} expectation
* - nums {string || array of strings}
* - name {string || array of strings}
* - axis {string}
* @param {string} msg
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks - I find that a lot easier, especially as you point out since this lives in a different file from where it gets called.

*/
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],
expectation.nums,
ptMsg + ' (nums)'
);
assertLabelContent(
d3.select(ptSelector + '> text.name'),
ptExpectation[1],
expectation.name,
ptMsg + ' (name)'
);
} else if(ptCnt > 1) {
expect(ptCnt).toBe(ptExpectation.length, ptMsg + ' # of visible nodes');
if(!Array.isArray(expectation.nums) || !Array.isArray(expectation.name)) {
fail(ptMsg + ': expecting more than 1 labels.');
}

expect(ptCnt)
.toBe(expectation.name.length, ptMsg + ' # of visible labels');

d3.selectAll(ptSelector).each(function(_, i) {
assertLabelContent(
d3.select(this).select('text.nums'),
ptExpectation[i][0],
expectation.nums[i],
ptMsg + ' (nums ' + i + ')'
);
assertLabelContent(
d3.select(this).sele 8000 ct('text.name'),
ptExpectation[i][1],
expectation.name[i],
ptMsg + ' (name ' + i + ')'
);
});
} else {
expect(ptExpectation).toBe(null, ptMsg + ' should not be displayed');
if(expectation.nums) {
fail(ptMsg + ': expecting *nums* labels, did not find any.');
}
if(expectation.name) {
fail(ptMsg + ': expecting *nums* labels, did not find any.');
}
}

if(axCnt > 0) {
if(axCnt) {
assertLabelContent(
d3.select(axSelector + '> text'),
axExpectation,
expectation.axis,
axMsg
);
} else {
expect(axExpectation).toBe(null, axMsg + ' should not be displayed');
if(expectation.axis) {
fail(axMsg + ': expecting label, did not find any.');
}
}
};

Expand Down
5 changes: 4 additions & 1 deletion test/jasmine/tests/carpet_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,7 +579,10 @@ describe('scattercarpet hover labels', function() {

return Plotly.plot(gd, fig).then(function() {
mouseEvent('mousemove', pos[0], pos[1]);
assertHoverLabelContent([content, null]);
assertHoverLabelContent({
nums: content[0].join('\n'),
name: content[1]
});
});
}

Expand Down
14 changes: 10 additions & 4 deletions test/jasmine/tests/choropleth_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,14 @@ describe('Test choropleth hover:', function() {

return Plotly.plot(gd, fig).then(function() {
mouseEvent('mousemove', pos[0], pos[1]);
assertHoverLabelContent([content, null]);
assertHoverLabelStyle(d3.select('g.hovertext'), style);
assertHoverLabelContent({
nums: content[0],
name: content[1]
});
assertHoverLabelStyle(
d3.select('g.hovertext'),
style
);
});
}

Expand All @@ -88,7 +94,7 @@ describe('Test choropleth hover:', function() {
run(
[400, 160],
fig,
[['RUS', '10', ''], 'trace 1']
['RUS\n10', 'trace 1']
)
.then(done);
});
Expand Down Expand Up @@ -130,7 +136,7 @@ describe('Test choropleth hover:', function() {
run(
[400, 160],
fig,
[['RUS', '10', ''], 'trace 1'],
['RUS\n10', 'trace 1'],
{
bgcolor: 'rgb(255, 0, 0)',
bordercolor: 'rgb(0, 128, 0)',
Expand Down
5 changes: 4 additions & 1 deletion test/jasmine/tests/gl2d_click_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,10 @@ describe('Test hover and click interactions', function() {
assertHoverLabelStyle(g, expected, opts.msg);
}
if(expected.label) {
assertHoverLabelContent([expected.label, null]);
assertHoverLabelContent({
nums: expected.label[0],
name: expected.label[1]
});
}
})
.then(_click)
Expand Down
2 changes: 1 addition & 1 deletion test/jasmine/tests/gl_plot_interact_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe('Test gl3d plots', function() {
function assertHoverText(xLabel, yLabel, zLabel, textLabel) {
var content = [xLabel, yLabel, zLabel];
if(textLabel) content.push(textLabel);
assertHoverLabelContent([[content, null], null]);
assertHoverLabelContent({nums: content.join('\n')});
}

function assertEventData(x, y, z, curveNumber, pointNumber, extra) {
Expand Down
Loading
0