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

Surface additional checks for handling empty z arrays and minimum number of rows and columns #3365

Merged
merged 4 commits into from Dec 21, 2018

Conversation

Projects
None yet
2 participants
@archmoj
Copy link
Collaborator

commented Dec 21, 2018

Fixes #3364.
The surface trace would be invisible if the z array is defined but is empty or when the columns/row array includes less than two items.
@etpinard

mock and fix for surface plot with empty z array
required minimum 2x2 data for a surface

revised conditions for z

correct z length check

new baseline added
@@ -0,0 +1,33 @@
{
"data": [

This comment has been minimized.

Copy link
@etpinard

etpinard Dec 21, 2018

Member

Could we turn these test cases into jasmine tests instead? This surface_test.js block

describe('supplyDefaults', function() {

should be a nice place to test the default logic.

var z = coerce('z');
if(!z) {
if(!z || !z.length ||
(x ? (x.length < 2) : false) ||

This comment has been minimized.

Copy link
@etpinard

etpinard Dec 21, 2018

Member

I would prefer keeping things consistent with contour traces here. 1-item contour traces are (like 1-item surfaces) ill-defined, but still appear as visible: true in the fullData (see -> https://codepen.io/etpinard/pen/oJZyEO)

To avoid drawing errors, we should handle the 1-item case in surface/convert.js. Thanks!

This comment has been minimized.

Copy link
@archmoj

archmoj Dec 21, 2018

Author Collaborator

The minimum for rows and columns are set to be one.
And it seems we won't get errors for those 1-item cases.

@etpinard

This comment has been minimized.

Copy link
Member

commented Dec 21, 2018

Nicely done 💃

@archmoj archmoj merged commit cbaae9a into master Dec 21, 2018

8 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: publish 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
continuous-integration/appveyor/branch AppVeyor build succeeded
Details

@archmoj archmoj deleted the fix3364-surface-check-xyz branch Dec 21, 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.