You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/07/25 18:00:55 UTC
[GitHub] [superset] AAfghahi commented on a diff in pull request #20852: feat(SqlLab): Change Save Dataset Button to Split Save Query Button IV
AAfghahi commented on code in PR #20852:
URL: https://github.com/apache/superset/pull/20852#discussion_r929150695
##########
superset-frontend/src/SqlLab/components/ResultSet/index.tsx:
##########
@@ -29,19 +29,14 @@ import {
SaveDatasetModal,
} from 'src/SqlLab/components/SaveDatasetModal';
import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
-import { EXPLORE_CHART_DEFAULT } from 'src/SqlLab/types';
Review Comment:
I think this was lost in a rebase potentially? Please revert this.
##########
superset-frontend/src/components/Chart/Chart.jsx:
##########
@@ -136,6 +139,12 @@ class Chart extends React.PureComponent {
}
}
+ toggleSaveDatasetModal = () => {
Review Comment:
same
##########
superset-frontend/src/SqlLab/components/SaveQuery/index.tsx:
##########
@@ -118,9 +129,7 @@ export default function SaveQuery({
setDescription(e.target.value);
};
- const toggleSave = () => {
- setShowSave(!showSave);
- };
+ const toggleSave = () => setShowSave(!showSave);
Review Comment:
nit: I think that this would be better taking in a boolean value that we set. Sometimes when connections are slower, the action doesn't immediately render and users click multiple times, causing it to reverse back to original value.
##########
superset-frontend/src/components/Chart/Chart.jsx:
##########
@@ -122,8 +123,10 @@ const MonospaceDiv = styled.div`
class Chart extends React.PureComponent {
constructor(props) {
super(props);
+ this.state = { showSaveDatasetModal: false };
Review Comment:
same here
##########
superset-frontend/src/SqlLab/components/ResultSet/index.tsx:
##########
@@ -275,11 +250,19 @@ export default class ResultSet extends React.PureComponent<
this.props.database?.allows_virtual_table_explore && (
<ExploreResultsButton
database={this.props.database}
- onClick={() => this.setState({ showSaveDatasetModal: true })}
+ onClick={() => {
+ // There is currently redux / state issue where sometimes a query will have serverId
+ // and other times it will not. We need this attribute consistently for this to work
+ // const qid = this.props?.query?.results?.query_id;
+ // if (qid) {
+ // // This will open explore using the query as datasource
+ // window.location.href = `/explore/?dataset_type=query&dataset_id=${qid}`;
+ // } else {
+ // this.setState({ showSaveDatasetModal: true });
+ // }
+ this.setState({ showSaveDatasetModal: true });
+ }}
/>
- // In order to use the new workflow for a query powered chart, replace the
Review Comment:
more of a broader question, when do we make this functionality the default behavior?
##########
superset-frontend/src/components/Chart/Chart.jsx:
##########
@@ -253,6 +262,7 @@ class Chart extends React.PureComponent {
width,
} = this.props;
+ const { showSaveDatasetModal } = this.state;
Review Comment:
same as above
##########
superset-frontend/src/components/Chart/ChartErrorMessage.tsx:
##########
@@ -42,6 +42,5 @@ export const ChartErrorMessage: React.FC<Props> = ({
...error,
extra: { ...error.extra, owners },
};
-
Review Comment:
nit: add this space back?
##########
superset-frontend/src/SqlLab/components/ResultSet/index.tsx:
##########
@@ -218,26 +213,6 @@ export default class ResultSet extends React.PureComponent<
}
}
- createExploreResultsOnClick = async () => {
Review Comment:
Also this
##########
superset-frontend/src/components/Chart/Chart.jsx:
##########
@@ -25,6 +25,7 @@ import { PLACEHOLDER_DATASOURCE } from 'src/dashboard/constants';
import Loading from 'src/components/Loading';
import { EmptyStateBig } from 'src/components/EmptyState';
import ErrorBoundary from 'src/components/ErrorBoundary';
+import { SaveDatasetModal } from 'src/SqlLab/components/SaveDatasetModal';
Review Comment:
I believe that this is part of an older rebase, and is no longer in this portion of the code base.
##########
superset-frontend/src/components/Chart/Chart.jsx:
##########
@@ -296,27 +306,39 @@ class Chart extends React.PureComponent {
}
return (
- <ErrorBoundary
- onError={this.handleRenderContainerFailure}
- showMessage={false}
- >
- <Styles
- data-ui-anchor="chart"
- className="chart-container"
- data-test="chart-container"
- height={height}
- width={width}
+ <>
+ <ErrorBoundary
+ onError={this.handleRenderContainerFailure}
+ showMessage={false}
>
- <div className="slice_container" data-test="slice-container">
- <ChartRenderer
- {...this.props}
- source={this.props.dashboardId ? 'dashboard' : 'explore'}
- data-test={this.props.vizType}
- />
- </div>
- {isLoading && !isDeactivatedViz && <Loading />}
- </Styles>
- </ErrorBoundary>
+ <Styles
+ data-ui-anchor="chart"
+ className="chart-container"
+ data-test="chart-container"
+ height={height}
+ width={width}
+ >
+ <div className="slice_container" data-test="slice-container">
+ <ChartRenderer
+ {...this.props}
+ source={this.props.dashboardId ? 'dashboard' : 'explore'}
+ data-test={this.props.vizType}
+ />
+ </div>
+ {isLoading && !isDeactivatedViz && <Loading />}
+ </Styles>
+ </ErrorBoundary>
+ {showSaveDatasetModal && (
+ <SaveDatasetModal
+ key={Math.random()}
Review Comment:
I think the SaveDatasetModal can be deleted from this, we ended up invoking it in another portion of the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org