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

Conversation

Projects
None yet
2 participants
@archmoj
Copy link
Collaborator

commented Oct 18, 2018

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

@alexcjohnson

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

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.

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

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Oct 18, 2018

Contributor

@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'))

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Oct 18, 2018

Contributor

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

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 18, 2018

Thank you @alexcjohnson
💃

@alexcjohnson alexcjohnson merged commit 1a075ff into master Oct 18, 2018

6 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-image2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine2 Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details

@alexcjohnson alexcjohnson deleted the display-parcoords-context branch Oct 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.