Skip to content

Skip undefined props on restyle #844

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

Merged
merged 2 commits into from
Aug 11, 2016
Merged

Skip undefined props on restyle #844

merged 2 commits into from
Aug 11, 2016

Conversation

rreusser
Copy link
Contributor

This PR allows the following syntax:

Plotly.restyle(gd, {'marker.color': [null, undefined]}, [0, 1]);

which will restore trace 0 to its default and leave trace 1 unaffected. This is necessary for the animation restyle so that property changes can be merged into a single restyle. Otherwise it's not possible to avoid modifying undesired properties.

This PR amounts to a single continue in the restyle loop if the newVal is undefined. The extra checks in the loop don't appear to be relevant for this since they don't actually use the value.

@etpinard
Copy link
Contributor

nicely done 💃

@etpinard etpinard added this to the v1.17.0 milestone Aug 11, 2016
@rreusser
Copy link
Contributor Author

rreusser commented Aug 11, 2016

@etpinard Before merging can you foresee any problems? It's a very small change, but technically it's a change to the public API, so want to be sure it's safe.

(In other words, just because it doesn't break tests doesn't mean it doesn't break existing use of plotly.js)

@etpinard
Copy link
Contributor

etpinard commented Aug 11, 2016

@rreusser Now, that you mention, we should make sure that the undefined are skipped in relayout too.

Other then that, I have no problem with this PR.

@rreusser
Copy link
Contributor Author

@etpinard updated to include relayout (tested via setting/unsetting width)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants