First working version of the geoboundingbox refine for the TileMap #2712
Conversation
| } else if (!!(filter.meta) && !!(filter.meta.index)) { | ||
| return filter.meta.index === this.id; | ||
| } else if (!!(filter.geo_bounding_box)) { | ||
| var field = _.keys(filter.geo_bounding_box)[0]; |
There was a problem hiding this comment.
You don't look for any particular type of filter in this function otherwise. Why is this required? Seems like if this is needed some level of abstraction should be added to allow for other filters that might need this functionality.
There was a problem hiding this comment.
This filtering is required because otherwise defining geo-bounding boxes on unknown fields raises query errors.
I tried to make this filtering as generic as possible so it applies to other kind of filters as well. Indeed refining all the queries of a dashboard with a drill down that exists only on some of the queries leads to empty widgets (as the filter won't match)
I agree you might want to make this a config option at the dashboard level, although I don't see any mechanism to do this in the current state of the code.
There was a problem hiding this comment.
Yeah, I don't particularly like the behavior change that is taking place here, nor that we're checking for some specific filter type. Also, what is being done with that field variable?
|
Can you add some details about the implementation? I'm confused why updates to the abstract data source are required to add a new filter type. |
|
Also there don't appear to be any tests for the code added here. |
|
Thank you Rashid for your feedback. |
| @@ -7,7 +7,7 @@ define(function (require) { | |||
|
|
|||
| return function (vis) { | |||
| var data = new Data(vis.data, vis._attr); | |||
|
|
|||
| data.color = function (label) {return '#ff6128'; }; | |||
There was a problem hiding this comment.
Whats with the hardcoded color here? As far as I can tell this isn't used anywhere else in this pull?
|
I haven't seen any feedback on this and we're trying to clean up the pull list for release. I'm going to go ahead and close the pull but feel free to reopen when you're ready. |
This should fix issue #2035
TODO #2034