Skip to content
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.

Sign up for GitHub

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

feat: Use SPA navigation between AddSlice and Dataset list pages #21683

Merged
merged 1 commit into from
Oct 4, 2022
Merged

feat: Use SPA navigation between AddSlice and Dataset list pages #21683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions superset-frontend/src/addSlice/AddSliceContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { styled, t, SupersetClient, JsonResponse } from '@superset-ui/core';
import { getUrlParam } from 'src/utils/urlUtils';
import { URL_PARAMS } from 'src/constants';
import { isNullish } from 'src/utils/common';
import { withRouter, RouteComponentProps } from 'react-router-dom';
import { Link, withRouter, RouteComponentProps } from 'react-router-dom';
import Button from 'src/components/Button';
import { AsyncSelect, Steps } from 'src/components';
import { Tooltip } from 'src/components/Tooltip';
Expand All @@ -32,7 +32,6 @@ import withToasts from 'src/components/MessageToasts/withToasts';
import VizTypeGallery, {
MAX_ADVISABLE_VIZ_GALLERY_WIDTH,
} from 'src/explore/components/controls/VizTypeControl/VizTypeGallery';
import _ from 'lodash';
import { findPermission } from 'src/utils/findPermission';
import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';

Expand Down Expand Up @@ -329,18 +328,18 @@ export class AddSliceContainer extends React.PureComponent<
const isButtonDisabled = this.isBtnDisabled();
const datasetHelpText = this.state.canCreateDataset ? (
<span data-test="dataset-write">
<a
href="/tablemodelview/list/#create"
rel="noopener noreferrer"
target="_blank"
<Link
to="/tablemodelview/list/#create"
data-test="add-chart-new-dataset"
>
{t('Add a dataset')}
</a>
</Link>
{` ${t('or')} `}
<a
href="https://superset.apache.org/docs/creating-charts-dashboards/creating-your-first-dashboard/#registering-a-new-table"
rel="noopener noreferrer"
target="_blank"
data-test="add-chart-new-dataset-instructions"
>
{`${t('view instructions')} `}
<i className="fa fa-external-link" />
Expand Down
14 changes: 10 additions & 4 deletions superset-frontend/src/views/CRUD/data/dataset/AddDatasetModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* under the License.
*/
import React, { FunctionComponent, useState, useEffect } from 'react';
import { useHistory } from 'react-router-dom';
import { styled, t } from '@superset-ui/core';
import { useSingleViewResource } from 'src/views/CRUD/hooks';
import Modal from 'src/components/Modal';
Expand Down Expand Up @@ -54,6 +55,7 @@ const DatasetModal: FunctionComponent<DatasetModalProps> = ({
onHide,
show,
}) => {
const history = useHistory();
const [currentDatabase, setCurrentDatabase] = useState<
DatabaseObject | undefined
>();
Expand Down Expand Up @@ -100,9 +102,13 @@ const DatasetModal: FunctionComponent<DatasetModalProps> = ({
setDisableSave(true);
};

const hide = () => {
setItem(LocalStorageKeys.db, null);
const cleanup = () => {
clearModal();
setItem(LocalStorageKeys.db, null);
};

const hide = () => {
cleanup();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I separated the cleanup functions from onHide(), because onHide does history.replace, which causes race condition with history.push in line 131

onHide();
};

Expand All @@ -122,8 +128,8 @@ const DatasetModal: FunctionComponent<DatasetModalProps> = ({
if (onDatasetAdd) {
onDatasetAdd({ id: response.id, ...response });
}
window.location.href = `/chart/add?dataset=${currentTableName}`;
hide();
history.push(`/chart/add?dataset=${currentTableName}`);
cleanup();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous version had a call to onHide. Should we keep it?

const hide = () => {
    setItem(LocalStorageKeys.db, null);
    clearModal();
    onHide();
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, onHide only executed history.replace, which was supposed to remove the #create from URL. We only want it when we close the modal, otherwise it causes a race condition with history.push from line 131 (explained in https://github.com/apache/superset/pull/21683/files#r985828109)

});
};

Expand Down