Skip to content

Make frame with nulls clear items & array containers #1118

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 11 commits into from
Nov 11, 2016
Merged

Make frame with nulls clear items & array containers #1118

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
69bffe1
test: add cases for Lib.expandObjectPaths w/ array containers
etpinard Nov 7, 2016
4c49755
make clean data work for non-plain-object annotation / shape items
etpinard Nov 9, 2016
d21d782
add general array container defaults handler
etpinard Nov 9, 2016
138ac35
DRY layout array container defaults using handleArrayContainerDefaults
etpinard Nov 9, 2016
d0953a0
test: add cases for array container defaults
etpinard Nov 9, 2016
174b640
make sure array containers set to null in frames are honored
etpinard Nov 9, 2016
36859f1
make sure items inside array container set to null are honored in frames
etpinard Nov 9, 2016
66f8bb7
test: add cases for extendObjectWithContainers
etpinard Nov 9, 2016
8ed1957
Merge branch 'master' into frame-extend-with-nulls
etpinard Nov 10, 2016
9de2f77
make annotation/shapes cleanLayout blocks more robust
etpinard Nov 10, 2016
5de6ff0
split container opts from item opts in handleArrayContainerDefaults
etpinard Nov 10, 2016
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
15 changes: 7 additions & 8 deletions src/plot_api/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,13 +102,12 @@ exports.cleanLayout = function(layout) {
}
}

if(layout.annotations !== undefined && !Array.isArray(layout.annotations)) {
Lib.warn('Annotations must be an array.');
delete layout.annotations;
}
var annotationsLen = (layout.annotations || []).length;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this commit was enough to fix bug discovered in http://codepen.io/etpinard/pen/xRGgwW

Copy link
Collaborator

Choose a reason for hiding this comment

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

in handleArrayContainerDefaults you do Array.isArray(parentObjIn[name]) ? parentObjIn[name] : [] - you want to do that here too so we still keep going if annotations isn't even an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch here. Thanks!

The only (I think) case where cont || [] vs Array.isArray(cont) ? ... matters is when someone inputs a string instead of an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, true.length doesn't break 😮

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 9de2f77

for(i = 0; i < annotationsLen; i++) {
var ann = layout.annotations[i];

if(!Lib.isPlainObject(ann)) continue;

if(ann.ref) {
if(ann.ref === 'paper') {
ann.xref = 'paper';
Expand All @@ -120,17 +119,17 @@ exports.cleanLayout = function(layout) {
}
delete ann.ref;
}

cleanAxRef(ann, 'xref');
cleanAxRef(ann, 'yref');
}

if(layout.shapes !== undefined && !Array.isArray(layout.shapes)) {
Lib.warn('Shapes must be an array.');
delete layout.shapes;
}
var shapesLen = (layout.shapes || []).length;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Array.isArray again... I guess if you test this situation you may find even more of these...

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 9de2f77

for(i = 0; i < shapesLen; i++) {
var shape = layout.shapes[i];

if(!Lib.isPlainObject(shape)) continue;

cleanAxRef(shape, 'xref');
cleanAxRef(shape, 'yref');
}
Expand Down
14 changes: 14 additions & 0 deletions test/jasmine/tests/lib_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,20 @@ describe('Test lib.js:', function() {
expect(computed).toLooseDeepEqual(expected);
});

it('does not skip over array container set to null values', function() {
var input = {title: 'clear annotations', annotations: null};
var expected = {title: 'clear annotations', annotations: null};
var computed = Lib.expandObjectPaths(input);
expect(computed).toLooseDeepEqual(expected);
});

it('expands array containers', function() {
var input = {title: 'clear annotation 1', 'annotations[1]': { title: 'new' }};
var expected = {title: 'clear annotation 1', annotations: [null, { title: 'new' }]};
var computed = Lib.expandObjectPaths(input);
expect(computed).toLooseDeepEqual(expected);
});

// TODO: This test is unimplemented since it's a currently-unused corner case.
// Getting the test to pass requires some extension (pun?) to extendDeepNoArrays
// that's intelligent enough to only selectively merge *some* arrays, in particular
Expand Down