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

Source Filters #2165

Merged
merged 35 commits into from
Jul 10, 2018
Merged

Source Filters #2165

merged 35 commits into from
Jul 10, 2018

Conversation

jesusbotella
Copy link
Contributor

This PR contains Carto.JS source filters. This will allow people to filter rows out of their sources, either SQL or Dataset sources.

There are two kind of filters:

  • carto.filter.Range
  • carto.filter.Category

And two filter collections, that join child filters in a different way:

  • carto.filter.OR
  • carto.filter.AND

All the filters have different parameters, related with the filter type, so that you can filter rows according to the column type.

You can use filters and filters collections as you wish, creating all the filters that you can think of, before and after adding them to a source. These filters are live updated, so whenever you change the filters, the visualization will be updated automatically applying the defined filters.

If you want to know more about the API definition, go to #2141 or read the docs.

This PR includes documentation and filter examples.

@@ -0,0 +1,172 @@
const _ = require('underscore');
const Base = require('./base');
const getObjectValue = require('../../../../src/util/get-object-value');
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add Wepback aliases since we're adding ES6 also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commit and take ownership 😂

@@ -17,9 +18,12 @@ var CartoValidationError = require('../error-handling/carto-validation-error');
* @api
*/
function Dataset (tableName) {
Base.apply(this, arguments);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call this before validating the tableName as it was before?

@@ -19,17 +19,20 @@ var CartoError = require('../error-handling/carto-error');
* @api
*/
function SQL (query) {
Base.apply(this, arguments);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, should we call this before checking that query is valid?

this._internalModel.set('query', query, { silent: true });

return this._internalModel._engine.reload()
.then(() => this._triggerQueryChanged(this, query))
Copy link
Contributor

Choose a reason for hiding this comment

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

So beautiful with the arrow functions 😍

},
'invalid-filter': {
messageRegex: /invalidFilter(.+)/,
// TODO: Add link to documentation?
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do it?

@jesusbotella
Copy link
Contributor Author

I have a proposal to improve error messages because they can be much better than now.

https://github.com/CartoDB/carto.js/pull/2165/files/22497bec4bdf3c1693f99661a82ed79c804ee554#diff-f93df9e087fbb4571b84c55dce17abf3R208
I wanted to put the allowed attributes there but I don't know how to pass them to print them in the string, for example. 🤷‍♂️

My proposal is to include a function in validation-errors that render the error message if exists, and if there's not any function it'll use friendlyMessage property.

@jesusbotella jesusbotella changed the base branch from update-webpack4 to master July 9, 2018 08:51
@rubenmoya
Copy link
Contributor

I've been messing around a little bit with the filters and I think it would be nice to have some helpers like getFilters() and hasFilter(myFilter), what do you think?

@jesusbotella jesusbotella merged commit 32f9826 into master Jul 10, 2018
@jesusbotella jesusbotella deleted the 2141-source-filters branch July 10, 2018 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants