-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
Added filter in ControlPanelsContainer for explore V2 #1647
Conversation
3a11f6a
to
ecd45cc
Compare
// only display filters with current prefix | ||
if (filter.prefix === this.props.prefix) { | ||
filters.push( | ||
<div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we pull this out into it's own SFC?
}); | ||
} | ||
/* eslint no-param-reassign: 0 */ | ||
delete form_data[`${prefix}_col_${i}`]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to delete these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I store and deal with filters in store as arrays since it's cleaner, in the form_data passed in it has arbitrary number of flt_eq/op/col as params, if we don't delete them they will linger in store and passed back again in onQuery.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok cool
return filters; | ||
}; | ||
|
||
const getFilters = function (form_data, datasource_type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
our style guide calls for named functions:
function getFilters(form_data, datasource_type) {
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}, | ||
{ | ||
label: 'Result Filters', | ||
description: 'Filters are defined using comma delimited strings as in <US,FR,Other>' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is Result Filters
supposed to have the same description as Filters
?
// Todo: remove after launch | ||
data.V2 = true; | ||
data.datasource_id = form_data.datasource; | ||
data.datasource_type = datasource_type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could simplify this to:
const data = {
datasource_type,
datasource_id: form_data.datasource,
V2: true, // todo: remove after launch, V2 tag temporarily for updating url
};
@@ -7,6 +7,7 @@ import { Panel, Alert } from 'react-bootstrap'; | |||
import { visTypes, commonControlPanelSections } from '../stores/store'; | |||
import ControlPanelSection from './ControlPanelSection'; | |||
import FieldSetRow from './FieldSetRow'; | |||
import Filters from './Filters'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have a test for this component? if not in this PR, would probably be beneficial to add.
0c70a9c
to
7d9308f
Compare
603c3c5
to
e1172b3
Compare
e1172b3
to
da104ae
Compare
Done:
Todo:
needs-review @ascott @mistercrunch