-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
apply redux for VisualizeModal #2795
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,10 @@ export const REMOVE_DATA_PREVIEW = 'REMOVE_DATA_PREVIEW'; | |
export const CHANGE_DATA_PREVIEW_ID = 'CHANGE_DATA_PREVIEW_ID'; | ||
export const SAVE_QUERY = 'SAVE_QUERY'; | ||
|
||
export const CREATE_DATASOURCE_STARTED = 'CREATE_DATASOURCE_STARTED'; | ||
export const CREATE_DATASOURCE_SUCCESS = 'CREATE_DATASOURCE_SUCCESS'; | ||
export const CREATE_DATASOURCE_FAILED = 'CREATE_DATASOURCE_FAILED'; | ||
|
||
export function resetState() { | ||
return { type: RESET_STATE }; | ||
} | ||
|
@@ -382,3 +386,38 @@ export function popSavedQuery(saveQueryId) { | |
}); | ||
}; | ||
} | ||
|
||
export function createDatasourceStarted() { | ||
return { type: CREATE_DATASOURCE_STARTED }; | ||
} | ||
export function createDatasourceSuccess(response) { | ||
const data = JSON.parse(response); | ||
const datasource = `${data.table_id}__table`; | ||
return { type: CREATE_DATASOURCE_SUCCESS, datasource }; | ||
} | ||
export function createDatasourceFailed(err) { | ||
return { type: CREATE_DATASOURCE_FAILED, err }; | ||
} | ||
|
||
export function createDatasource(vizOptions, context) { | ||
return (dispatch) => { | ||
dispatch(createDatasourceStarted()); | ||
|
||
return $.ajax({ | ||
type: 'POST', | ||
url: '/superset/sqllab_viz/', | ||
async: false, | ||
data: { | ||
data: JSON.stringify(vizOptions), | ||
}, | ||
context, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i don't think we need to set context here, since we are not using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i will need 'this' in the chained done/fail |
||
dataType: 'json', | ||
success: (resp) => { | ||
dispatch(createDatasourceSuccess(resp)); | ||
}, | ||
error: () => { | ||
dispatch(createDatasourceFailed('An error occurred while creating the data source')); | ||
}, | ||
}); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,15 @@ | ||
/* global notify */ | ||
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import { bindActionCreators } from 'redux'; | ||
import { connect } from 'react-redux'; | ||
import { Alert, Button, Col, Modal } from 'react-bootstrap'; | ||
|
||
import Select from 'react-select'; | ||
import { Table } from 'reactable'; | ||
import shortid from 'shortid'; | ||
import $ from 'jquery'; | ||
import { getExploreUrl } from '../../explorev2/exploreUtils'; | ||
import * as actions from '../actions'; | ||
|
||
const CHART_TYPES = [ | ||
{ value: 'dist_bar', label: 'Distribution - Bar Chart', requiresTime: false }, | ||
|
@@ -17,9 +19,12 @@ const CHART_TYPES = [ | |
]; | ||
|
||
const propTypes = { | ||
actions: PropTypes.object.isRequired, | ||
onHide: PropTypes.func, | ||
query: PropTypes.object, | ||
show: PropTypes.bool, | ||
datasource: PropTypes.string, | ||
errorMessage: PropTypes.string, | ||
}; | ||
const defaultProps = { | ||
show: false, | ||
|
@@ -121,22 +126,14 @@ class VisualizeModal extends React.PureComponent { | |
sql: this.props.query.sql, | ||
dbId: this.props.query.dbId, | ||
}; | ||
notify.info('Creating a data source and popping a new tab'); | ||
$.ajax({ | ||
type: 'POST', | ||
url: '/superset/sqllab_viz/', | ||
async: false, | ||
data: { | ||
data: JSON.stringify(vizOptions), | ||
}, | ||
dataType: 'json', | ||
success: (resp) => { | ||
|
||
this.props.actions.createDatasource(vizOptions, this) | ||
.done(() => { | ||
const columns = Object.keys(this.state.columns).map(k => this.state.columns[k]); | ||
const data = JSON.parse(resp); | ||
const mainMetric = columns.filter(d => d.agg)[0]; | ||
const mainGroupBy = columns.filter(d => d.is_dim)[0]; | ||
const formData = { | ||
datasource: `${data.table_id}__table`, | ||
datasource: this.props.datasource, | ||
viz_type: this.state.chartType.value, | ||
since: '100 years ago', | ||
limit: '0', | ||
|
@@ -148,14 +145,16 @@ class VisualizeModal extends React.PureComponent { | |
if (mainGroupBy) { | ||
formData.groupby = [mainGroupBy.name]; | ||
} | ||
notify.info('Creating a data source and popping a new tab'); | ||
|
||
window.open(getExploreUrl(formData)); | ||
}, | ||
error: () => notify('An error occurred while creating the data source'), | ||
}); | ||
}) | ||
.fail(() => { | ||
notify.error(this.props.errorMessage); | ||
}); | ||
} | ||
changeDatasourceName(event) { | ||
this.setState({ datasourceName: event.target.value }); | ||
this.validate(); | ||
this.setState({ datasourceName: event.target.value }, this.validate); | ||
} | ||
changeCheckbox(attr, columnName, event) { | ||
let columns = this.mergedColumns(); | ||
|
@@ -271,4 +270,19 @@ class VisualizeModal extends React.PureComponent { | |
VisualizeModal.propTypes = propTypes; | ||
VisualizeModal.defaultProps = defaultProps; | ||
|
||
export default VisualizeModal; | ||
function mapStateToProps(state) { | ||
return { | ||
datasource: state.datasource, | ||
errorMessage: state.errorMessage, | ||
}; | ||
} | ||
|
||
function mapDispatchToProps(dispatch) { | ||
return { | ||
actions: bindActionCreators(actions, dispatch), | ||
}; | ||
} | ||
|
||
export { VisualizeModal }; | ||
export default connect(mapStateToProps, mapDispatchToProps)(VisualizeModal); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the general approach is to minimize the number of components that interact directly with redux. Components that do should be suffixed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we aren't really using a consistent pattern around this yet. currently the following components in SQLLab are connected to redux/mapping state to props: for this component, we could pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. About 'When to Introduce Containers?', I feel this article is reasonable: Presentational and Container Components. For VisulizeModal component, I feel its state (datasource, and errorMessage) are rarely needed by other components. If we make it a dump component, since VisulizeModal is used by QueryTable and ResultSet, does it means both parents need to pass additional pros down? So I prefer to wrap its own states in a container. Currently SqlLab code structure is more like:
I will prefer to have it breakdown into
I am still new to this code base, would like to hear your opinions. Thank you! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's a good article about presentational and container components. there are a lot of good points there, and i agree that it's an ideal architecture, separating out data fetching/updating logic from pure presentational components. i think it would be worth doing a deep dive on the the component/redux architecture of sql lab, to see where we might optimize/organize which components will be containers, and which will be presentational. right now we have a bit of a mix happening. i would say connecting to redux from the visualizeModal component for now works, and allows us to better test that component by making that change. let's leave re-organizing for ideal container/presentation components for a follow up PR. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -205,3 +205,7 @@ div.widget .slice_container { | |
.table-condensed { | ||
font-size: 12px; | ||
} | ||
|
||
.table-condensed input[type="checkbox"] { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should probably scope this to the visualize modal only. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. i will move it to another PR. |
||
float: left; | ||
} |
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 don't think we want/need to use
aync: false
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.
it was in the original code:
https://github.com/airbnb/superset/blob/ac3aba7c7da905f0f4bfeb0f80efb87136812b90/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx
But i also think it is not necessary...Will remove it.