Skip to content
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

Filter fixes #1062

Merged
merged 7 commits into from
Oct 24, 2016
Merged

Filter fixes #1062

merged 7 commits into from
Oct 24, 2016

Conversation

etpinard
Copy link
Contributor

After first being released in v1.18.0, we come to the conclusion that filter transforms are incompatible with plot.ly's webapp data references.

So, we have no choice but to 🔪 the filtersrc attribute and rename it 'target'. A backward-compatible block has been to cleanData.

Moreover, to allow filtering about arbitrary columns of data, we now allow 'target' to be an arbitrary array of data - which required a few patches to the Lib.findArrayAttributes.

cc @rreusser @bpostlethwaite @chriddyp

@etpinard etpinard added bug something broken feature something new status: reviewable labels Oct 20, 2016
@etpinard etpinard added this to the v1.19.0 milestone Oct 20, 2016
var transform = transforms[i];

stack = ['transforms[' + i + ']'];
exports.crawl(transform._module.attributes, callback, 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loop through transform container found in the trace, use the transform module attributes declaration to determine which attributes are data_array or arrayOk.

}]
})]);

expect(out[0].x).toEqual([-1, 1, 2]);
expect(out[0].y).toEqual([2, 2, 3]);
expect(out[0].ids).toEqual(['n1', 'p1', 'p2']);
});

describe('filters should handle array *target* values', function() {
Copy link
Contributor Author

@etpinard etpinard Oct 20, 2016

Choose a reason for hiding this comment

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

@chriddyp please review the lines ⏬

thank you.

Copy link
Member

Choose a reason for hiding this comment

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

looks good!

layout: {}
};

Plots.supplyDefaults(mockGd);
Copy link
Contributor Author

@etpinard etpinard Oct 20, 2016

Choose a reason for hiding this comment

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

Calling the full supplyDefaults here is an overkill.

@monfera factored the autoType routine in 5b71b47, I might just use that instead once #1021 is merged.

@etpinard
Copy link
Contributor Author

Oh and by the way @bpostlethwaite transform attribute are already part of the plot schema:

image

@bpostlethwaite
Copy link
Member

bpostlethwaite commented Oct 21, 2016

👍!
targetsrc doesn't need to be part of Plotly.js. I think we can safely look at removing all the *src keys from Plotlyjs attributes.

💃 from me!

@etpinard etpinard merged commit e397429 into master Oct 24, 2016
@etpinard etpinard deleted the filter-fixes branch October 24, 2016 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants