From f6f40c815a9ed962d5bbbf9dd4fb357479bd3677 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian <1858430+suddjian@users.noreply.github.com> Date: Fri, 13 Mar 2020 15:14:50 -0700 Subject: [PATCH] [Charts] Use the Edit Properties modal throughout React views (#9267) * typescriptification * use the chart edit modal on the react list view * linting * typings don't work on old react-bootstrap version * lint * remove duplicate field --- superset-frontend/.eslintrc.js | 1 + superset-frontend/package-lock.json | 59 ++++++++++++++ superset-frontend/package.json | 2 + .../components/DisplayQueryButton_spec.jsx | 2 +- .../components/ExploreChartHeader_spec.jsx | 2 +- .../explore/components/DisplayQueryButton.jsx | 25 ++++-- .../explore/components/ExploreChartHeader.jsx | 48 ++++++++++-- ...ropertiesModal.jsx => PropertiesModal.tsx} | 78 ++++++++++++------- superset-frontend/src/types/Chart.ts | 33 ++++++++ .../src/views/chartList/ChartList.tsx | 60 ++++++++++---- 10 files changed, 255 insertions(+), 55 deletions(-) rename superset-frontend/src/explore/components/{PropertiesModal.jsx => PropertiesModal.tsx} (80%) create mode 100644 superset-frontend/src/types/Chart.ts diff --git a/superset-frontend/.eslintrc.js b/superset-frontend/.eslintrc.js index f8ec0027f..d73326abc 100644 --- a/superset-frontend/.eslintrc.js +++ b/superset-frontend/.eslintrc.js @@ -48,6 +48,7 @@ module.exports = { properties: 'never', }, ], + '@typescript-eslint/no-empty-function': 0, '@typescript-eslint/no-explicit-any': 0, '@typescript-eslint/explicit-function-return-type': 0, camelcase: 0, diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json index c7a93d591..90759e8ae 100644 --- a/superset-frontend/package-lock.json +++ b/superset-frontend/package-lock.json @@ -5031,6 +5031,56 @@ "@types/webpack": "*" } }, + "@types/react-redux": { + "version": "7.1.7", + "resolved": "https://registry.npmjs.org/@types/react-redux/-/react-redux-7.1.7.tgz", + "integrity": "sha512-U+WrzeFfI83+evZE2dkZ/oF/1vjIYgqrb5dGgedkqVV8HEfDFujNgWCwHL89TDuWKb47U0nTBT6PLGq4IIogWg==", + "dev": true, + "requires": { + "@types/hoist-non-react-statics": "^3.3.0", + "@types/react": "*", + "hoist-non-react-statics": "^3.3.0", + "redux": "^4.0.0" + }, + "dependencies": { + "hoist-non-react-statics": { + "version": "3.3.2", + "resolved": "https://registry.npmjs.org/hoist-non-react-statics/-/hoist-non-react-statics-3.3.2.tgz", + "integrity": "sha512-/gGivxi8JPKWNm/W0jSmzcMPpfpPLc3dY/6GxhX2hQ9iGj3aDfklV4ET7NjKpSinLpJ5vafa9iiGIEZg10SfBw==", + "dev": true, + "requires": { + "react-is": "^16.7.0" + } + }, + "react-is": { + "version": "16.13.0", + "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.13.0.tgz", + "integrity": "sha512-GFMtL0vHkiBv9HluwNZTggSn/sCyEt9n02aM0dSAjGGyqyNlAyftYm4phPxdvCigG15JreC5biwxCgTAJZ7yAA==", + "dev": true + }, + "redux": { + "version": "4.0.5", + "resolved": "https://registry.npmjs.org/redux/-/redux-4.0.5.tgz", + "integrity": "sha512-VSz1uMAH24DM6MF72vcojpYPtrTUu3ByVWfPL1nPfVRb5mZVTve5GnNCUV53QM/BZ66xfWrm0CTWoM+Xlz8V1w==", + "dev": true, + "requires": { + "loose-envify": "^1.4.0", + "symbol-observable": "^1.2.0" + } + } + } + }, + "@types/react-select": { + "version": "3.0.10", + "resolved": "https://registry.npmjs.org/@types/react-select/-/react-select-3.0.10.tgz", + "integrity": "sha512-oUHXqvbkRhC07q5JjeY6hE+NUqgUM6CyaRXEKYPvMCBqUOuLnYltyhiNx6Jpb+iFpYtNHSQtF4dNJfMdMooKoQ==", + "dev": true, + "requires": { + "@types/react": "*", + "@types/react-dom": "*", + "@types/react-transition-group": "*" + } + }, "@types/react-table": { "version": "7.0.2", "resolved": "https://registry.npmjs.org/@types/react-table/-/react-table-7.0.2.tgz", @@ -5040,6 +5090,15 @@ "@types/react": "*" } }, + "@types/react-transition-group": { + "version": "4.2.4", + "resolved": "https://registry.npmjs.org/@types/react-transition-group/-/react-transition-group-4.2.4.tgz", + "integrity": "sha512-8DMUaDqh0S70TjkqU0DxOu80tFUiiaS9rxkWip/nb7gtvAsbqOXm02UCmR8zdcjWujgeYPiPNTVpVpKzUDotwA==", + "dev": true, + "requires": { + "@types/react": "*" + } + }, "@types/react-virtualized": { "version": "9.21.8", "resolved": "https://registry.npmjs.org/@types/react-virtualized/-/react-virtualized-9.21.8.tgz", diff --git a/superset-frontend/package.json b/superset-frontend/package.json index 323c19f1b..06e56c19f 100644 --- a/superset-frontend/package.json +++ b/superset-frontend/package.json @@ -174,6 +174,8 @@ "@types/react": "^16.4.18", "@types/react-dom": "^16.0.9", "@types/react-json-tree": "^0.6.11", + "@types/react-redux": "^7.1.7", + "@types/react-select": "^3.0.10", "@types/react-table": "^7.0.2", "@typescript-eslint/eslint-plugin": "^2.20.0", "@typescript-eslint/parser": "^2.20.0", diff --git a/superset-frontend/spec/javascripts/explore/components/DisplayQueryButton_spec.jsx b/superset-frontend/spec/javascripts/explore/components/DisplayQueryButton_spec.jsx index f2403541f..da369610b 100644 --- a/superset-frontend/spec/javascripts/explore/components/DisplayQueryButton_spec.jsx +++ b/superset-frontend/spec/javascripts/explore/components/DisplayQueryButton_spec.jsx @@ -20,7 +20,7 @@ import React from 'react'; import { mount } from 'enzyme'; import ModalTrigger from './../../../../src/components/ModalTrigger'; -import DisplayQueryButton from '../../../../src/explore/components/DisplayQueryButton'; +import { DisplayQueryButton } from '../../../../src/explore/components/DisplayQueryButton'; describe('DisplayQueryButton', () => { const defaultProps = { diff --git a/superset-frontend/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx b/superset-frontend/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx index 72e03c5f2..8ee9273f6 100644 --- a/superset-frontend/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx +++ b/superset-frontend/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx @@ -19,7 +19,7 @@ import React from 'react'; import { shallow } from 'enzyme'; -import ExploreChartHeader from '../../../../src/explore/components/ExploreChartHeader'; +import { ExploreChartHeader } from '../../../../src/explore/components/ExploreChartHeader'; import ExploreActionButtons from '../../../../src/explore/components/ExploreActionButtons'; import EditableTitle from '../../../../src/components/EditableTitle'; diff --git a/superset-frontend/src/explore/components/DisplayQueryButton.jsx b/superset-frontend/src/explore/components/DisplayQueryButton.jsx index 0c087721c..72759b16c 100644 --- a/superset-frontend/src/explore/components/DisplayQueryButton.jsx +++ b/superset-frontend/src/explore/components/DisplayQueryButton.jsx @@ -17,6 +17,8 @@ * under the License. */ import React from 'react'; +import { connect } from 'react-redux'; +import { bindActionCreators } from 'redux'; import PropTypes from 'prop-types'; import SyntaxHighlighter, { registerLanguage, @@ -47,6 +49,7 @@ import Button from '../../components/Button'; import RowCountLabel from './RowCountLabel'; import { prepareCopyToClipboardTabularData } from '../../utils/common'; import PropertiesModal from './PropertiesModal'; +import { sliceUpdated } from '../actions/exploreActions'; registerLanguage('markdown', markdownSyntax); registerLanguage('html', htmlSyntax); @@ -65,7 +68,7 @@ const defaultProps = { animation: true, }; -export default class DisplayQueryButton extends React.PureComponent { +export class DisplayQueryButton extends React.PureComponent { constructor(props) { super(props); const { datasource } = props.latestQueryFormData; @@ -220,6 +223,7 @@ export default class DisplayQueryButton extends React.PureComponent { return null; } render() { + const { animation, slice } = this.props; return ( - {this.props.slice && ( + {slice && ( <> {t('Edit properties')} )} {t('View query')}} modalTitle={t('View query')} bsSize="large" @@ -257,7 +262,7 @@ export default class DisplayQueryButton extends React.PureComponent { /> {t('View results')}} modalTitle={t('View results')} bsSize="large" @@ -266,7 +271,7 @@ export default class DisplayQueryButton extends React.PureComponent { /> {t('View samples')}} modalTitle={t('View samples')} bsSize="large" @@ -285,3 +290,9 @@ export default class DisplayQueryButton extends React.PureComponent { DisplayQueryButton.propTypes = propTypes; DisplayQueryButton.defaultProps = defaultProps; + +function mapDispatchToProps(dispatch) { + return bindActionCreators({ sliceUpdated }, dispatch); +} + +export default connect(null, mapDispatchToProps)(DisplayQueryButton); diff --git a/superset-frontend/src/explore/components/ExploreChartHeader.jsx b/superset-frontend/src/explore/components/ExploreChartHeader.jsx index 11989c6e3..a73b6fda5 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader.jsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader.jsx @@ -17,6 +17,8 @@ * under the License. */ import React from 'react'; +import { connect } from 'react-redux'; +import { bindActionCreators } from 'redux'; import PropTypes from 'prop-types'; import { t } from '@superset-ui/translation'; @@ -29,6 +31,8 @@ import FaveStar from '../../components/FaveStar'; import TooltipWrapper from '../../components/TooltipWrapper'; import Timer from '../../components/Timer'; import CachedLabel from '../../components/CachedLabel'; +import PropertiesModal from './PropertiesModal'; +import { sliceUpdated } from '../actions/exploreActions'; const CHART_STATUS_MAP = { failed: 'danger', @@ -49,7 +53,16 @@ const propTypes = { chart: chartPropShape, }; -class ExploreChartHeader extends React.PureComponent { +export class ExploreChartHeader extends React.PureComponent { + constructor(props) { + super(props); + this.state = { + isPropertiesModalOpen: false, + }; + this.openProperiesModal = this.openProperiesModal.bind(this); + this.closePropertiesModal = this.closePropertiesModal.bind(this); + } + postChartFormData() { this.props.actions.postChartFormData( this.props.form_data, @@ -93,6 +106,18 @@ class ExploreChartHeader extends React.PureComponent { }); } + openProperiesModal() { + this.setState({ + isPropertiesModalOpen: true, + }); + } + + closePropertiesModal() { + this.setState({ + isPropertiesModalOpen: false, + }); + } + renderChartTitle() { let title; if (this.props.slice) { @@ -131,17 +156,24 @@ class ExploreChartHeader extends React.PureComponent { saveFaveStar={this.props.actions.saveFaveStar} isStarred={this.props.isStarred} /> - + - - + )} @@ -187,4 +219,8 @@ class ExploreChartHeader extends React.PureComponent { ExploreChartHeader.propTypes = propTypes; -export default ExploreChartHeader; +function mapDispatchToProps(dispatch) { + return bindActionCreators({ sliceUpdated }, dispatch); +} + +export default connect(null, mapDispatchToProps)(ExploreChartHeader); diff --git a/superset-frontend/src/explore/components/PropertiesModal.jsx b/superset-frontend/src/explore/components/PropertiesModal.tsx similarity index 80% rename from superset-frontend/src/explore/components/PropertiesModal.jsx rename to superset-frontend/src/explore/components/PropertiesModal.tsx index 9424fd6da..8e292f07d 100644 --- a/superset-frontend/src/explore/components/PropertiesModal.jsx +++ b/superset-frontend/src/explore/components/PropertiesModal.tsx @@ -17,8 +17,6 @@ * under the License. */ import React, { useState, useEffect, useRef } from 'react'; -import { connect } from 'react-redux'; -import { bindActionCreators } from 'redux'; import { Button, Modal, @@ -26,16 +24,41 @@ import { Col, FormControl, FormGroup, + // @ts-ignore } from 'react-bootstrap'; +// @ts-ignore import Dialog from 'react-bootstrap-dialog'; import Select from 'react-select'; import { t } from '@superset-ui/translation'; -import { SupersetClient } from '@superset-ui/connection'; - -import { sliceUpdated } from '../actions/exploreActions'; +import { SupersetClient, Json } from '@superset-ui/connection'; +import Chart from 'src/types/Chart'; import getClientErrorObject from '../../utils/getClientErrorObject'; -function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) { +export type Slice = { + slice_id: number; + slice_name: string; + description: string | null; + cache_timeout: number | null; +}; + +type InternalProps = { + slice: Slice; + onHide: () => void; + onSave: (chart: Chart) => void; +}; + +export type WrapperProps = InternalProps & { + show: boolean; + animation?: boolean; // for the modal +}; + +export default function PropertiesModalWrapper({ + show, + onHide, + animation, + slice, + onSave, +}: WrapperProps) { // The wrapper is a separate component so that hooks only run when the modal opens return ( @@ -44,9 +67,9 @@ function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) { ); } -function PropertiesModal({ slice, onHide, onSave }) { +function PropertiesModal({ slice, onHide, onSave }: InternalProps) { const [submitting, setSubmitting] = useState(false); - const errorDialog = useRef(); + const errorDialog = useRef(null); const [ownerOptions, setOwnerOptions] = useState(null); // values of form inputs @@ -55,9 +78,9 @@ function PropertiesModal({ slice, onHide, onSave }) { const [cacheTimeout, setCacheTimeout] = useState( slice.cache_timeout != null ? slice.cache_timeout : '', ); - const [owners, setOwners] = useState(null); + const [owners, setOwners] = useState(null); - function showError({ error, statusText }) { + function showError({ error, statusText }: any) { errorDialog.current.show({ title: 'Error', bsSize: 'medium', @@ -72,8 +95,9 @@ function PropertiesModal({ slice, onHide, onSave }) { const response = await SupersetClient.get({ endpoint: `/api/v1/chart/${slice.slice_id}`, }); + const chart = (response.json as Json).result; setOwners( - response.json.result.owners.map(owner => ({ + chart.owners.map((owner: any) => ({ value: owner.id, label: owner.username, })), @@ -94,8 +118,9 @@ function PropertiesModal({ slice, onHide, onSave }) { SupersetClient.get({ endpoint: `/api/v1/chart/related/owners`, }).then(res => { + const { result } = res.json as Json; setOwnerOptions( - res.json.result.map(item => ({ + result.map((item: any) => ({ value: item.value, label: item.text, })), @@ -103,7 +128,7 @@ function PropertiesModal({ slice, onHide, onSave }) { }); }, []); - const onSubmit = async event => { + const onSubmit = async (event: React.FormEvent) => { event.stopPropagation(); event.preventDefault(); setSubmitting(true); @@ -111,7 +136,7 @@ function PropertiesModal({ slice, onHide, onSave }) { slice_name: name || null, description: description || null, cache_timeout: cacheTimeout || null, - owners: owners.map(o => o.value), + owners: owners!.map(o => o.value), }; try { const res = await SupersetClient.put({ @@ -120,7 +145,11 @@ function PropertiesModal({ slice, onHide, onSave }) { body: JSON.stringify(payload), }); // update the redux state - onSave(res.json.result); + const updatedChart = { + ...(res.json as Json).result, + id: slice.slice_id, + }; + onSave(updatedChart); onHide(); } catch (res) { const clientError = await getClientErrorObject(res); @@ -147,7 +176,9 @@ function PropertiesModal({ slice, onHide, onSave }) { type="text" bsSize="sm" value={name} - onChange={event => setName(event.target.value)} + onChange={(event: React.ChangeEvent) => + setName(event.target.value) + } /> @@ -160,7 +191,9 @@ function PropertiesModal({ slice, onHide, onSave }) { componentClass="textarea" bsSize="sm" value={description} - onChange={event => setDescription(event.target.value)} + onChange={(event: React.ChangeEvent) => + setDescription(event.target.value) + } style={{ maxWidth: '100%' }} />

@@ -181,7 +214,7 @@ function PropertiesModal({ slice, onHide, onSave }) { type="text" bsSize="sm" value={cacheTimeout} - onChange={event => + onChange={(event: React.ChangeEvent) => setCacheTimeout(event.target.value.replace(/[^0-9]/, '')) } /> @@ -218,7 +251,7 @@ function PropertiesModal({ slice, onHide, onSave }) { bsSize="sm" bsStyle="primary" className="m-r-5" - disabled={submitting} + disabled={!owners || submitting} > {t('Save')} @@ -230,10 +263,3 @@ function PropertiesModal({ slice, onHide, onSave }) { ); } - -function mapDispatchToProps(dispatch) { - return bindActionCreators({ onSave: sliceUpdated }, dispatch); -} - -export { PropertiesModalWrapper }; -export default connect(null, mapDispatchToProps)(PropertiesModalWrapper); diff --git a/superset-frontend/src/types/Chart.ts b/superset-frontend/src/types/Chart.ts new file mode 100644 index 000000000..9b7433751 --- /dev/null +++ b/superset-frontend/src/types/Chart.ts @@ -0,0 +1,33 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * The Chart model as returned from the API + */ + +export default interface Chart { + id: number; + url: string; + viz_type: string; + slice_name: string; + creator: string; + changed_on: string; + description: string | null; + cache_timeout: number | null; +} diff --git a/superset-frontend/src/views/chartList/ChartList.tsx b/superset-frontend/src/views/chartList/ChartList.tsx index 95f734a0f..ebf1a484a 100644 --- a/superset-frontend/src/views/chartList/ChartList.tsx +++ b/superset-frontend/src/views/chartList/ChartList.tsx @@ -31,6 +31,8 @@ import { Filters, } from 'src/components/ListView/types'; import withToasts from 'src/messageToasts/enhancers/withToasts'; +import PropertiesModal, { Slice } from 'src/explore/components/PropertiesModal'; +import Chart from 'src/types/Chart'; const PAGE_SIZE = 25; @@ -48,15 +50,9 @@ interface State { owners: Array<{ text: string; value: number }>; lastFetchDataConfig: FetchDataConfig | null; permissions: string[]; -} - -interface Chart { - changed_on: string; - creator: string; - id: number; - slice_name: string; - url: string; - viz_type: string; + // for now we need to use the Slice type defined in PropertiesModal. + // In future it would be better to have a unified Chart entity. + sliceCurrentlyEditing: Slice | null; } class ChartList extends React.PureComponent { @@ -73,6 +69,7 @@ class ChartList extends React.PureComponent { loading: false, owners: [], permissions: [], + sliceCurrentlyEditing: null, }; componentDidMount() { @@ -181,7 +178,7 @@ class ChartList extends React.PureComponent { { Cell: ({ row: { state, original } }: any) => { const handleDelete = () => this.handleChartDelete(original); - const handleEdit = () => this.handleChartEdit(original); + const openEditModal = () => this.openChartEditModal(original); if (!this.canEdit && !this.canDelete) { return null; } @@ -218,7 +215,7 @@ class ChartList extends React.PureComponent { role="button" tabIndex={0} className="action-button" - onClick={handleEdit} + onClick={openEditModal} > @@ -239,8 +236,29 @@ class ChartList extends React.PureComponent { return this.state.permissions.some(p => p === perm); }; - handleChartEdit = ({ id }: { id: number }) => { - window.location.assign(`/chart/edit/${id}`); + openChartEditModal = (chart: Chart) => { + this.setState({ + sliceCurrentlyEditing: { + slice_id: chart.id, + slice_name: chart.slice_name, + description: chart.description, + cache_timeout: chart.cache_timeout, + }, + }); + }; + + closeChartEditModal = () => { + this.setState({ sliceCurrentlyEditing: null }); + }; + + handleChartUpdated = (edits: Chart) => { + // update the chart in our state with the edited info + const newCharts = this.state.charts.map(chart => + chart.id === edits.id ? { ...chart, ...edits } : chart, + ); + this.setState({ + charts: newCharts, + }); }; handleChartDelete = ({ id, slice_name: sliceName }: Chart) => { @@ -367,10 +385,24 @@ class ChartList extends React.PureComponent { }; render() { - const { charts, chartCount, loading, filters } = this.state; + const { + charts, + chartCount, + loading, + filters, + sliceCurrentlyEditing, + } = this.state; return (

+ {sliceCurrentlyEditing && ( + + )}