Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Consistent container update / removal #1086

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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Oct 26, 2016
Merged

Consistent container update / removal #1086

merged 5 commits into from
Oct 26, 2016

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

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

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

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

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

you have [annotation2]

That is, null clears items in the array containers.

@bpostlethwaite
Copy link
Member

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

馃拑 for the PR. Just a few questions

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

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

@rreusser rreusser rreusser left review comments

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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.