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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
