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

Merged
merged 2 commits into from
Mar 31, 2017
Merged

Assume recalc is needed when restyling attribute containers #1536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af15b34
assume redoing calcdata is neccessary when restyling attr containers
etpinard Mar 30, 2017
e10c57f
:books: for redo calcdata when restyling array values
etpinard Mar 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/plot_api/plot_api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

flags.docalc = true;
}

// must redo calcdata when restyling array values of arrayOk attributes
if(valObject.arrayOk && (Array.isArray(newVal) || Array.isArray(oldVal))) {
flags.docalc = true;
}
Expand Down
19 changes: 19 additions & 0 deletions test/jasmine/tests/plot_api_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,25 @@ describe('Test plot api', function() {
expect(PlotlyInternal.plot).toHaveBeenCalled();
});

it('should do full replot when attribute container are updated', function() {
var gd = {
data: [{x: [1, 2, 3], y: [1, 2, 3]}],
layout: {
xaxis: { range: [0, 4] },
yaxis: { range: [0, 4] }
}
};

mockDefaultsAndCalc(gd);
Plotly.restyle(gd, {
marker: {
color: ['red', 'blue', 'green']
}
});
expect(gd.calcdata).toBeUndefined();
expect(PlotlyInternal.plot).toHaveBeenCalled();
});

it('calls plot on xgap and ygap styling', function() {
var gd = {
data: [{z: [[1, 2, 3], [4, 5, 6], [7, 8, 9]], showscale: false, type: 'heatmap'}],
Expand Down