-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Array edits #1403
Merged
+1,688
−698
Merged
Array edits #1403
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
cc45189
fix basic annotation reference change bug
alexcjohnson 13a87ce
support RegExp in Lib.pushUnique
alexcjohnson 61f250c
editContainerArray: consistent handling of container array edits
alexcjohnson a9526bf
test fixin
alexcjohnson 649a831
robustify click_test
alexcjohnson d55568c
:hocho: obsolete getShapeLayer
alexcjohnson 2e6c030
break up new circular deps
alexcjohnson f7e60fb
relinkPrivateKeys inside arrayContainerDefaults
alexcjohnson d7cdc0a
revert layout_image.json - keep #1390 separate for now
alexcjohnson 7ed9eb9
fix for components using category names as coordinates
alexcjohnson 9f89dd6
fix for autorange with bad axis refs from a component
alexcjohnson 46962af
point to open issue #1111
alexcjohnson 1af058a
allow all `info_array`s to be pruned and command args to be optional
alexcjohnson c6378b1
exclude `args` from `nestedProperty` pruning
alexcjohnson 70c882b
keep _fullLayout container arrays in sync with splices in layout
alexcjohnson 5fb462f
comments and tests for plot_api/helpers.hasParent
alexcjohnson d29cbae
editContainerArray => applyContainerArrayChanges
alexcjohnson 3ef1c9b
test errors from parent/child simultaneous edits
alexcjohnson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -375,6 +375,30 @@ describe('Test lib.js:', function() { | |
expect(obj).toEqual({a: [undefined, {}]}); | ||
}); | ||
|
||
it('does not prune inside `args` arrays', function() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nicely done. Thanks! |
||
var obj = {}, | ||
args = np(obj, 'args'); | ||
|
||
args.set([]); | ||
expect(obj.args).toBeUndefined(); | ||
|
||
args.set([null]); | ||
expect(obj.args).toEqual([null]); | ||
|
||
np(obj, 'args[1]').set([]); | ||
expect(obj.args).toEqual([null, []]); | ||
|
||
np(obj, 'args[2]').set({}); | ||
expect(obj.args).toEqual([null, [], {}]); | ||
|
||
np(obj, 'args[1]').set(); | ||
expect(obj.args).toEqual([null, undefined, {}]); | ||
|
||
// we still trim undefined off the end of arrays, but nothing else. | ||
np(obj, 'args[2]').set(); | ||
expect(obj.args).toEqual([null]); | ||
}); | ||
|
||
it('should get empty, and fail on set, with a bad input object', function() { | ||
var badProps = [ | ||
np(5, 'a'), | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work for
domain.x
anddomain.y
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And more generally, shouldn't all
info_array
attributes (e.g. updemenues/slidersargs
, andgeo.projection.parallels
) be deletable?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good point. I was trying to match the previous logic and extend it gently but honestly I can't recall anymore why we had that logic - why does an empty data array need to stick around while other empty arrays can be removed entirely? Perhaps just to keep things simple for streaming? Does that point to a more robust condition on what should be deletable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, letting every array be deletable and running the full test suite was fairly enlightening - There are a lot of things that fail or error out, some of which could be addressed but fundamentally for data arrays there really is a difference between
[]
(which means there is data of that kind, it's just presently empty) andundefined
(which means the data/attribute should get some default value filled in).As far as I can tell, for
info_array
attributes there is no difference between[]
andundefined
though. So unless we want to try and build in here some way to hunt through the schema (which sounds super painful to me, and potentially not even well-defined given thatnestedProperty
can, on occasion, be called on a root object that's neither a complete trace or layout object but some portion of it) I would argue for some simple tests that might frequently describe an info array as falsely not deletable but will never describe a real data array as deletable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expanded the safe-to-delete list to include all
info_array
attributes in 1af058a@rreusser this also required making
args
optional for sliders and updatemenus (fallback to[]
in command.js) since an empty array will now get pruned. Not sure there will be a real use case for this (what method could one call with no args?) but this case pops up in the tests anyhow 🏆 and it does seem more plotly-esque to not require empty parameters.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only use case I'm aware of is animating an empty list of frames
[]
which (serendipitously) functions as a pause button. I've had a bit of trouble with this behavior, so we documented pretty clearly in the web docs that you must use[null]
instead of[]
to prevent it from getting filtered and replaced with a barenull
.That is, in particular, it's when you create an updatemenus button with a command that pauses. It filters
updatemenus[0].buttons[0].args = [[], {...}]
and replaces it with[null, {...}]
in that case.