-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Explore control panel - Chart control, TimeFilter, GroupBy, Filters #1205
Explore control panel - Chart control, TimeFilter, GroupBy, Filters #1205
Conversation
dcfb604
to
b67906c
Compare
* create structure for new forked explore view * update component name * add bootstrap data pattern * remove console.log
* Created store and reducers * Added spec * Modifications based on comments
* Associate version to entry files * Modified path joins for configs * Made changes based on comments
50c5a95
to
29df846
Compare
02cc616
to
f80bae7
Compare
GroupBy and Filters
f80bae7
to
ee61d17
Compare
export const SET_ROW_LIMIT = 'SET_ROW_LIMIT'; | ||
export const TOGGLE_SEARCHBOX = 'TOGGLE_SEARCHBOX'; | ||
export const SET_FILTER_COLUMN_OPTS = 'SET_FILTER_COLUMN_OPTS'; | ||
export const SET_WHERE_CLAUSE = 'SET_SET_WHERE_CLAUSE'; |
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.
extra SET_
in value
export const TOGGLE_SEARCHBOX = 'TOGGLE_SEARCHBOX'; | ||
export const SET_FILTER_COLUMN_OPTS = 'SET_FILTER_COLUMN_OPTS'; | ||
export const SET_WHERE_CLAUSE = 'SET_SET_WHERE_CLAUSE'; | ||
export const SET_HAVING_CLAUSE = 'SET_SET_HAVING_CLAUSE'; |
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.
same here
const timeGrainOpts = []; | ||
|
||
if (datasourceId) { | ||
const params = []; |
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 params = [`datasource_id=${datasourceId}`, `datasource_type=${datasourceType}`];
import React from 'react'; | ||
import Select from 'react-select'; | ||
import { bindActionCreators } from 'redux'; | ||
import * as actions from '../actions/exploreActions'; |
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.
rather than importing all actions here i would suggest just importing the ones needed for this component...
import { setFormOpts, setDatasource, resetFormData, setVizType }
as actions from '../actions/exploreActions';
} | ||
|
||
ChartControl.propTypes = { | ||
actions: React.PropTypes.object, |
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 think it makes more sense to import the actions in this component rather than passing them in as props.
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 saw Sqllab components were doing this, I think it's for mapDispatchToProps, so that we can bind action creators into one, then dispatch from the component props
class ChartControl extends React.Component { | ||
componentWillMount() { | ||
if (this.props.datasourceId) { | ||
this.props.actions.setFormOpts(this.props.datasourceId, this.props.datasourceType); |
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.
once we import actions above we can use them like this:
setFormOpts(this.props.datasourceId, this.props.datasourceType);
makes more sense to import the actions in this component rather than passing them in as props.
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.
This didn't work.. Suggested by the redux doc here: http://redux.js.org/docs/basics/Actions.html#action-creators
We should bind action creators to dispatch() function, then map dispatch to component's props. I could still explicitly import actions like:
import { setFormOpts, setDatasource, resetFormData, setVizType } from '../actions/exploreActions';
and then do this in mapDispatchToProps:
actions: bindActionCreators({ setFormOpts, setDatasource, resetFormData, setVizType, }, dispatch),
But dispatching actions would still be the same:
this.props.actions.setDatasource(val);
Is there a better way to do this?
@ascott
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.
ah ok, maybe i am carrying over some Alt patterns that don't work in redux. so when you call bindActionCreators
the actions are passed as props to the component?
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.
The way I understand it is that bindActionCreators bind all action creators with dispatch function, so that we don't have to write redundant onClick() => dispatch(actionFunc()) statements. mapDispatchToProps() allows us to call dispatch function from the component's props, instead of accessing global store everytime we dispatch.
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.
👍
return { type: CLEAR_ALL_OPTS }; | ||
} | ||
|
||
export function setFormOpts(datasourceId, datasourceType) { |
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 think this function belongs in an actionCreators
file but it we can change that in a subsequent pr
@expose("/fetch_datasource_metadata") | ||
@log_this | ||
def fetch_datasource_metadata(self): | ||
# TODO: check permissions |
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.
doesn't @has_access
do permission checking for us?
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 think it's only checking access to the endpoint, but we need access checking for datasource/slice here
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.
ah gotcha, good catch
return enhancer; | ||
} | ||
|
||
export const VIZ_TYPES = [ |
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 think this belongs in a explore/constants.js
file
|
||
import { exploreReducer } from './reducers/exploreReducer'; | ||
|
||
const metrics = []; |
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.
const metrics = [bootstrapData.viz.form_data.metric]
import * as actions from '../actions/exploreActions'; | ||
import { connect } from 'react-redux'; | ||
|
||
const sinceOptions = ['1 hour ago', '12 hours ago', '1 day ago', |
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 move these time constants to explore/constants.js
} | ||
|
||
TimeFilter.propTypes = { | ||
actions: React.PropTypes.object, |
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.
it's a nice pattern to put proptypes at the top of the file like this:
const propTypes = {
actions: React.PropTypes.object,
timeColumnOpts: React.PropTypes.array,
timeColumn: React.PropTypes.string,
timeGrainOpts: React.PropTypes.array,
timeGrain: React.PropTypes.string,
since: React.PropTypes.string,
until: React.PropTypes.string,
};
const defaultProps = {
...
};
and then here at the bottom:
TimeFilter.propTypes = propTypes;
TimeFilter.defaultProps = defaultProps;
<Select | ||
multi | ||
name="select-since" | ||
placeholder={'Select metrics'} |
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.
don't need curly brackets here
placeholder="Select metrics"
actions: React.PropTypes.object, | ||
}; | ||
|
||
SqlClause.defaultProps = { |
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.
we can delete this if there are no default props
<Select | ||
className="col-sm-6" | ||
name="select-time-column" | ||
placeholder={'Select a time column'} |
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.
placeholder="Select a time column"
import { createStore, applyMiddleware, compose } from 'redux'; | ||
import { Provider } from 'react-redux'; | ||
import thunk from 'redux-thunk'; | ||
import { getDevEnhancer } from '../../utils/common'; |
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 moved this and other redux utils to a common reduxUtils.js file https://github.com/ascott/caravel/blob/master/caravel/assets/javascripts/reduxUtils.js#L67
this generally looking good! i think we could improve the organization of our components, but that can be in a subsequent PR. seems like there maybe some conflicts with master, but we can resolve those once we merge this into another PR for the js tests would be 💯 🚢 |
2ec83db
to
6ad3bf7
Compare
🎉 |
* Explore control panel - Chart control, TimeFilter, GroupBy, Filters (#1205) * create structure for new forked explore view (#1099) * create structure for new forked explore view * update component name * add bootstrap data pattern * remove console.log * Associate version to entry files (#1060) * Associate version to entry files * Modified path joins for configs * Made changes based on comments * Created store and reducers (#1108) * Created store and reducers * Added spec * Modifications based on comments * Explore control panel components: Chart control, Time filter, SQL, GroupBy and Filters * Modifications based on comments * accommodate old and new explore urls * move bootstrap data up in scope * fix code climate issues * fix long lines * fix syntax error
…pache#1205) * create structure for new forked explore view (apache#1099) * create structure for new forked explore view * update component name * add bootstrap data pattern * remove console.log * Associate version to entry files (apache#1060) * Associate version to entry files * Modified path joins for configs * Made changes based on comments * Created store and reducers (apache#1108) * Created store and reducers * Added spec * Modifications based on comments * Explore control panel components: Chart control, Time filter, SQL, GroupBy and Filters * Modifications based on comments
* Explore control panel - Chart control, TimeFilter, GroupBy, Filters (#1205) * create structure for new forked explore view (#1099) * create structure for new forked explore view * update component name * add bootstrap data pattern * remove console.log * Associate version to entry files (#1060) * Associate version to entry files * Modified path joins for configs * Made changes based on comments * Created store and reducers (#1108) * Created store and reducers * Added spec * Modifications based on comments * Explore control panel components: Chart control, Time filter, SQL, GroupBy and Filters * Modifications based on comments * Added access check + Druid in endpoint * pull grains to constants * Switch explore.html to old version
…pache#1205) * create structure for new forked explore view (apache#1099) * create structure for new forked explore view * update component name * add bootstrap data pattern * remove console.log * Associate version to entry files (apache#1060) * Associate version to entry files * Modified path joins for configs * Made changes based on comments * Created store and reducers (apache#1108) * Created store and reducers * Added spec * Modifications based on comments * Explore control panel components: Chart control, Time filter, SQL, GroupBy and Filters * Modifications based on comments
…pache#1205) * fix(plugin-chart-echarts): x-filtering improvement for radar chart * fix(plugin-chart-echarts): opacity constants * fix(plugin-chart-echarts): improve constant * fix(plugin-chart-echarts): place treemap constant
…pache#1205) * fix(plugin-chart-echarts): x-filtering improvement for radar chart * fix(plugin-chart-echarts): opacity constants * fix(plugin-chart-echarts): improve constant * fix(plugin-chart-echarts): place treemap constant
…pache#1205) * fix(plugin-chart-echarts): x-filtering improvement for radar chart * fix(plugin-chart-echarts): opacity constants * fix(plugin-chart-echarts): improve constant * fix(plugin-chart-echarts): place treemap constant
…pache#1205) * fix(plugin-chart-echarts): x-filtering improvement for radar chart * fix(plugin-chart-echarts): opacity constants * fix(plugin-chart-echarts): improve constant * fix(plugin-chart-echarts): place treemap constant
Sorry I should have divided this to smaller PRs, will work on Todos in future smaller PR
needs-review @ascott
Done:
Todo:
Side note:
data for slice stored in { bootstrapData.viz.data }
data for pre-populating form stored in { bootstrapData.viz.form_data }