[FilterBox] dashboard date range filtering #1165
Conversation
| @@ -58,7 +58,7 @@ const px = function () { | |||
| const containerId = data.token + '_con'; | |||
| const selector = '#' + containerId; | |||
| const container = $(selector); | |||
| const sliceId = data.sliceId; | |||
| const sliceId = data.slice_id; | |||
There was a problem hiding this comment.
that's the last one!
|
|
||
| const propTypes = { | ||
| origSelectedValues: React.PropTypes.object, | ||
| filtersChoices: React.PropTypes.object, |
There was a problem hiding this comment.
is this a required prop? if so we should mark it isRequired
There was a problem hiding this comment.
it's not required, I added a default value for it
| @@ -10,22 +10,80 @@ import '../stylesheets/react-select/select.less'; | |||
|
|
|||
| import './filter_box.css'; | |||
|
|
|||
| const TIME_CHOICES = [ | |||
There was a problem hiding this comment.
could we put these in a constants.js file to be shared across the app?
There was a problem hiding this comment.
I'm putting the constants.js in viz folder to try to keep that contained somewhat
| onChange: () => {}, | ||
| showDateFilter: false, | ||
| }; | ||
|
|
||
| class FilterBox extends React.Component { |
There was a problem hiding this comment.
i think this component should be in it's own file in the components directory, and imported for use in this visualization file.
There was a problem hiding this comment.
I'd like to keep all of the visualization-specific as together as possible hoping that one day they can be standalone, or distributed outside of Caravel.
| '1 year ago', | ||
| ]; | ||
|
|
||
| const propTypes = { |
There was a problem hiding this comment.
i like how you moved these to the top of the file. it's a pattern i use too & find super useful when reading a component file. 👌
| changeFilter(filter, options) { | ||
| let vals = null; | ||
| if (options) { | ||
| if (Array.isArray(options)) { |
There was a problem hiding this comment.
why are options sometimes arrays and sometimes objects?
There was a problem hiding this comment.
because we use a mix of Select multiple and Select one... The time ones are single choice
| @@ -62,11 +62,11 @@ | |||
| "moments": "0.0.2", | |||
| "mustache": "^2.2.1", | |||
| "nvd3": "1.8.4", | |||
| "react": "^15.2.1", | |||
| "react": "^15.3.2", | |||
| @@ -205,36 +216,39 @@ def query_filters(self, is_having_filter=False): | |||
| if col and op and eq is not None: | |||
| filters.append((col, op, eq)) | |||
|
|
|||
| if is_having_filter: | |||
There was a problem hiding this comment.
is this a python-ism? would has_filter be more clear?
There was a problem hiding this comment.
it has to do with the SQL HAVING clause
|
Addressed/answered the comments, merging. |
mistercrunch
added
🏷️ bot
Adding functionality to FilterBox that allows setting global, dynamic time filters on dashboards that apply to all widgets (except the ones set to be immune to filtering).
fixes #894 #893 #665