Skip to content
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

Display parcoords deselected lines #3123

Merged
merged 3 commits into from
Oct 18, 2018
Merged

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Oct 18, 2018

This PR could fix the issue #3099 i.e. before the implementation of hovermode for parcoords.

@alexcjohnson

@alexcjohnson
Copy link
Collaborator

Looks great @archmoj ! Can you add a test to 🔒 this fix? I was going to suggest modeling it after this test:

d3.selectAll('.parcoords-lines').each(function(d) {
var imageArray = d.lineLayer.readPixels(0, 0, d.model.canvasWidth, d.model.canvasHeight);
var foundPixel = false;
var i = 0;
do {
foundPixel = foundPixel || imageArray[i++] !== 0;
} while(!foundPixel && i < imageArray.length);
expect(foundPixel).toEqual(false);
});

(with the inverse expectation of course, foundPixel should be true) And while I think the inner part of that test may still be good, the test itself is broken - it doesn't bother to test that d3.selectAll('.parcoords-lines') actually yields any elements! (add the line expect(d3.selectAll('.parcoords-lines').size()).not.toBe(0); and it fails) So the .each never runs. Must have been refactored so the .parcoords-lines class is never used.

Don't spend any time fixing that test now unless it's trivial, but please do come up with a similar test showing that all the layers have some data in them after an edit. No need to simulate clicking the modebar button, some other edit like the one I mentioned in #3099 Plotly.relayout(gd,'paper_bgcolor','#eff') will suffice.

also fix parcoords restyle test while we're at it
.then(function() {
return Plotly.relayout(gd, 'paper_bgcolor', '#eef');
})
.then(_assertVisibleData(true, 'after relayout'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

@archmoj before your patch this fails with:

Expected false to be true, 'after relayout - gl-canvas gl-canvas-context'.

Plotly.plot(gd, mockCopy)
.then(_assertVisibleData(true, 'initial'))
.then(restyleDimension('values', 1, [[]]))
.then(_assertVisibleData(false, 'no panels'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

previously this test didn't bother checking that it was actually doing anything! d3.selectAll('.parcoords-lines') was an empty set - this must have been refactored at some point - so the .each block never ran. Updated it to not only ensure that it's doing something (expect(canvases.size()).toBe(3, msg); up above) but also to test that something changed (_assertVisibleData(true, 'initial') before the change, _assertVisibleData(false, 'no panels') afterward).

@archmoj
Copy link
Contributor Author

archmoj commented Oct 18, 2018

Thank you @alexcjohnson
💃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants