Skip to content

Assume recalc is needed when restyling attribute containers #1536

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

Jump to bottom
Merged
merged 2 commits into from
Mar 31, 2017
Merged

Assume recalc is needed when restyling attribute containers #1536

merged 2 commits into from
Mar 31, 2017

Conversation

etpinard
Copy link
Contributor

fixes #1526

So that:

Plotly.restyle(gd, {
  marker: {
    color: ['red', 'blue', 'green']
  }
})

works in all situations.

@etpinard etpinard added status: reviewable bug something broken labels Mar 30, 2017
@etpinard etpinard added this to the v1.26.0 milestone Mar 30, 2017
@etpinard etpinard mentioned this pull request Mar 30, 2017
Closed
etpinard
etpinard commented Mar 30, 2017
View reviewed changes
src/plot_api/plot_api.js
@@ -1580,6 +1580,12 @@ function _restyle(gd, aobj, _traces) {
var moduleAttrs = (contFull._module || {}).attributes || {};
var valObject = Lib.nestedProperty(moduleAttrs, ai).get() || {};

// if restyling entire attribute container, assume worse case
if(!valObject.valType) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A little bit hacky as this relies on the fact that attribute containers (e.g. marker) don't have a valType field set in the attribute declarations.

Morevoer, we could optimize this further by digging into the attribute container for arrayOk keys, but I thought this wasn't necessary. For best-performing restyle calls, users should target specific attributes e.g. restyle(gd, 'marker.color', [['red', 'blue', 'green']]).

Copy link
Collaborator

Choose a reason for hiding this comment

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

if an attribute container did have a valType field, would (or could) the schema validation tests fail?

I agree 💯 it's not worth optimizing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would because if it had a valType field it would only need a valid role (i.e. set to 'info' | 'style' | 'data') field to pass this test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great, then I'm happy with this hack.

@alexcjohnson
Copy link
Collaborator

💃

@etpinard etpinard merged commit 627aeab into master Mar 31, 2017
@etpinard etpinard deleted the restyle-recalc-attr-containers branch March 31, 2017 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcjohnson alexcjohnson alexcjohnson left review comments

Assignees
No one assigned
Labels
bug something broken
Projects
None yet
Milestone
v1.26.0
Development

Successfully merging this pull request may close these issues.

BUG: After a 'pan' restyle function doesn't color points
2 participants
@etpinard @alexcjohnson