Skip to content

Consistent container update / removal #1086

Merged
etpinard merged 5 commits intomasterfrom
container-removal
Oct 26, 2016
Merged

Consistent container update / removal #1086
etpinard merged 5 commits intomasterfrom
container-removal

Conversation

@etpinard
Copy link
Contributor

@etpinard etpinard commented Oct 25, 2016 •
edited
Loading

fixes #1069

Currently, relayout is allowed to be called with 💠 special 💠 'add' and 'remove' values for annotations and shapes. For example,

Plotly.relayout(gd, 'annotations', 'add');
// => adds a blank annotations

Plotly.relayout(gd, 'annotations[2]', 'remove');
// => remove the 3rd annotation

which gets the job done, but is a little sloppy. We can do better and luckily 'add' and 'remove' we're never implemented for the new (e.g. images, updatemenus, sliders, mapbox layers) layout array containers.

This PR enforces,

Plotly.relayout(gd, `${containerName}[${itemIndex}]`, null);

as a way to delete items in containers found in layout. Similarly,

Plotly.restyle(gd, `transforms[${itemIndex}]`, null, [`${traceIndex}`]);

will delete transform item in the relevant trace(s).

Moreover,

Plotly.relayout(gd, `${containerName}[${newItemIndex}]`, {
  // container item opts 
});

will consistently add items to the array container if newItemIndex is beyond the current container length.

@etpinard etpinard added bug something broken feature something new status: in progress labels Oct 25, 2016
@etpinard etpinard added this to the v1.19.0 milestone Oct 25, 2016
@etpinard
Copy link
Contributor Author

etpinard commented Oct 25, 2016 •
edited
Loading

I made a PR right now even this is a WIP as this was the last remaning item in the v1.19.0 without a corresponding PR. I'll wrap this is up tomorrow.

TODO:

  • add mucho tests
  • add restyle logic for transforms

rreusser
rreusser reviewed Oct 25, 2016
View reviewed changes
src/plots/plots.js
'sliders',
'updatemenus'
]);
return plots.extendObjectWithContainers(destLayout, srcLayout, plots.layoutArrayContainers);
Copy link
Contributor

@rreusser rreusser Oct 25, 2016

Choose a reason for hiding this comment

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

Love it. Nice refactor.

Copy link
Contributor Author

@etpinard etpinard Oct 25, 2016

Choose a reason for hiding this comment

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

Yep. I thought exposing plots.layoutArrayContainers was a nice compromise.

Next step would be to auto-generate that list using the plot-schema. (later ...)

@rreusser
Copy link
Contributor

rreusser commented Oct 25, 2016

Outstanding! What happens to the surrounding indices when you do that? Presumably it's up to the user/front-end to manage references to non-existent array elements. Like if you have [annotation1, annotation2] and you remove the first annotation. Do you then have [null, annotation2] or [annotation2]?

@etpinard
Copy link
Contributor Author

etpinard commented Oct 26, 2016

Do you then have [null, annotation2] or [annotation2]?

you have [annotation2]

That is, null clears items in the array containers.

@bpostlethwaite
Copy link
Member

bpostlethwaite commented Oct 26, 2016

Is there a place we can add a deprecation notice about the 'add' and 'remove' API? Do any docs need updating? Should we add an issue in the Plotlyjs 2.0 to remove 'add' and 'remove' as part of that Milestone?

@bpostlethwaite
Copy link
Member

bpostlethwaite commented Oct 26, 2016

💃 for the PR. Just a few questions

@etpinard etpinard mentioned this pull request Oct 26, 2016
Closed
15 tasks
@etpinard
Copy link
Contributor Author

etpinard commented Oct 26, 2016

Is there a place we can add a deprecation notice about the 'add' and 'remove' API? Do any docs need updating?

I don't any reference of 'add' or 'remove' under https://plot.ly/javascript/#chart-events maybe @cldougl would know more?

Should we add an issue in the Plotlyjs 2.0 to remove 'add' and 'remove' as part of that Milestone?

Good call. I'll add it to #420

@etpinard etpinard merged commit ad864c4 into master Oct 26, 2016
@etpinard etpinard deleted the container-removal branch October 26, 2016 16:32
@rreusser
Copy link
Contributor

rreusser commented Oct 26, 2016 •
edited
Loading

@etpinard That makes sense. I'll only point out the obvious possibility of writing code that references other items by array index.

@cldougl cldougl mentioned this pull request Oct 26, 2016
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@rreusser rreusser rreusser left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug something broken feature something new

Projects

None yet

Milestone

v1.19.0

Development

Successfully merging this pull request may close these issues.

API for deleting transforms and other new Plotly.js components

3 participants

@etpinard @rreusser @bpostlethwaite