From 6b62d321118a40ead63e2e695253617008ec7337 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Tue, 19 Apr 2022 14:57:06 +0200 Subject: [PATCH] feat(explore): Replace overlay with alert banner when chart controls change (#19696) * Rename explore alert * Rename refreshOverlayVisible to chartIsStale * Implement banners * Add tests * Add clickable text to empty state * Fix viz type switching * styling changes * Fixes after rebasing * Code review fixes * Fix bug * Fix redundant refreshing --- .../src/components/Chart/Chart.jsx | 64 +++------ .../src/components/Chart/ChartRenderer.jsx | 28 ++-- .../components/Chart/ChartRenderer.test.jsx | 19 +-- .../explore/components/ControlPanelAlert.tsx | 98 -------------- .../components/ControlPanelsContainer.tsx | 7 +- .../src/explore/components/ExploreAlert.tsx | 127 ++++++++++++++++++ .../explore/components/ExploreChartPanel.jsx | 89 ++++++++++-- .../components/ExploreChartPanel.test.jsx | 75 +++++++++-- .../ExploreViewContainer.test.tsx | 5 +- .../components/ExploreViewContainer/index.jsx | 47 ++++--- .../controlUtils/getFormDataFromControls.ts | 7 +- .../getChartRequiredFieldsMissingMessage.ts | 26 ++++ 12 files changed, 375 insertions(+), 217 deletions(-) delete mode 100644 superset-frontend/src/explore/components/ControlPanelAlert.tsx create mode 100644 superset-frontend/src/explore/components/ExploreAlert.tsx create mode 100644 superset-frontend/src/utils/getChartRequiredFieldsMissingMessage.ts diff --git a/superset-frontend/src/components/Chart/Chart.jsx b/superset-frontend/src/components/Chart/Chart.jsx index 35209bb94af0..7df33d0c5d7c 100644 --- a/superset-frontend/src/components/Chart/Chart.jsx +++ b/superset-frontend/src/components/Chart/Chart.jsx @@ -22,7 +22,6 @@ import { styled, logging, t, ensureIsArray } from '@superset-ui/core'; import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags'; import { PLACEHOLDER_DATASOURCE } from 'src/dashboard/constants'; -import Button from 'src/components/Button'; import Loading from 'src/components/Loading'; import { EmptyStateBig } from 'src/components/EmptyState'; import ErrorBoundary from 'src/components/ErrorBoundary'; @@ -32,6 +31,7 @@ import { getUrlParam } from 'src/utils/urlUtils'; import { ResourceStatus } from 'src/hooks/apiResources/apiResources'; import ChartRenderer from './ChartRenderer'; import { ChartErrorMessage } from './ChartErrorMessage'; +import { getChartRequiredFieldsMissingMessage } from '../../utils/getChartRequiredFieldsMissingMessage'; const propTypes = { annotationData: PropTypes.object, @@ -64,7 +64,7 @@ const propTypes = { chartStackTrace: PropTypes.string, queriesResponse: PropTypes.arrayOf(PropTypes.object), triggerQuery: PropTypes.bool, - refreshOverlayVisible: PropTypes.bool, + chartIsStale: PropTypes.bool, errorMessage: PropTypes.node, // dashboard callbacks addFilter: PropTypes.func, @@ -108,20 +108,8 @@ const Styles = styled.div` } `; -const RefreshOverlayWrapper = styled.div` - position: absolute; - top: 0; - left: 0; - width: 100%; - height: 100%; - display: flex; - align-items: center; - justify-content: center; -`; - const MonospaceDiv = styled.div` font-family: ${({ theme }) => theme.typography.families.monospace}; - white-space: pre; word-break: break-word; overflow-x: auto; white-space: pre-wrap; @@ -255,34 +243,23 @@ class Chart extends React.PureComponent { chartAlert, chartStatus, errorMessage, - onQuery, - refreshOverlayVisible, + chartIsStale, queriesResponse = [], isDeactivatedViz = false, width, } = this.props; const isLoading = chartStatus === 'loading'; - const isFaded = refreshOverlayVisible && !errorMessage; this.renderContainerStartTime = Logger.getTimestamp(); if (chartStatus === 'failed') { return queriesResponse.map(item => this.renderErrorMessage(item)); } - if (errorMessage) { - const description = isFeatureEnabled( - FeatureFlag.ENABLE_EXPLORE_DRAG_AND_DROP, - ) - ? t( - 'Drag and drop values into highlighted field(s) on the left control panel and run query', - ) - : t( - 'Select values in highlighted field(s) on the left control panel and run query', - ); + if (errorMessage && ensureIsArray(queriesResponse).length === 0) { return ( ); @@ -291,15 +268,24 @@ class Chart extends React.PureComponent { if ( !isLoading && !chartAlert && - isFaded && + !errorMessage && + chartIsStale && ensureIsArray(queriesResponse).length === 0 ) { return ( + {t( + 'Click on "Create chart" button in the control panel on the left to preview a visualization or', + )}{' '} + + {t('click here')} + + . + + } image="chart.svg" /> ); @@ -317,25 +303,13 @@ class Chart extends React.PureComponent { height={height} width={width} > -
+
- - {!isLoading && !chartAlert && isFaded && ( - - - - )} - {isLoading && !isDeactivatedViz && } diff --git a/superset-frontend/src/components/Chart/ChartRenderer.jsx b/superset-frontend/src/components/Chart/ChartRenderer.jsx index 5b2f16d7f5eb..49c2577e07db 100644 --- a/superset-frontend/src/components/Chart/ChartRenderer.jsx +++ b/superset-frontend/src/components/Chart/ChartRenderer.jsx @@ -30,6 +30,7 @@ const propTypes = { datasource: PropTypes.object, initialValues: PropTypes.object, formData: PropTypes.object.isRequired, + latestQueryFormData: PropTypes.object, labelColors: PropTypes.object, sharedLabelColors: PropTypes.object, height: PropTypes.number, @@ -42,7 +43,7 @@ const propTypes = { chartStatus: PropTypes.string, queriesResponse: PropTypes.arrayOf(PropTypes.object), triggerQuery: PropTypes.bool, - refreshOverlayVisible: PropTypes.bool, + chartIsStale: PropTypes.bool, // dashboard callbacks addFilter: PropTypes.func, setDataMask: PropTypes.func, @@ -58,6 +59,8 @@ const BLANK = {}; const BIG_NO_RESULT_MIN_WIDTH = 300; const BIG_NO_RESULT_MIN_HEIGHT = 220; +const behaviors = [Behavior.INTERACTIVE_CHART]; + const defaultProps = { addFilter: () => BLANK, onFilterMenuOpen: () => BLANK, @@ -93,8 +96,7 @@ class ChartRenderer extends React.Component { const resultsReady = nextProps.queriesResponse && ['success', 'rendered'].indexOf(nextProps.chartStatus) > -1 && - !nextProps.queriesResponse?.[0]?.error && - !nextProps.refreshOverlayVisible; + !nextProps.queriesResponse?.[0]?.error; if (resultsReady) { this.hasQueryResponseChange = @@ -170,16 +172,10 @@ class ChartRenderer extends React.Component { } render() { - const { chartAlert, chartStatus, vizType, chartId, refreshOverlayVisible } = - this.props; + const { chartAlert, chartStatus, chartId } = this.props; // Skip chart rendering - if ( - refreshOverlayVisible || - chartStatus === 'loading' || - !!chartAlert || - chartStatus === null - ) { + if (chartStatus === 'loading' || !!chartAlert || chartStatus === null) { return null; } @@ -193,11 +189,17 @@ class ChartRenderer extends React.Component { initialValues, ownState, filterState, + chartIsStale, formData, + latestQueryFormData, queriesResponse, postTransformProps, } = this.props; + const currentFormData = + chartIsStale && latestQueryFormData ? latestQueryFormData : formData; + const vizType = currentFormData.viz_type || this.props.vizType; + // It's bad practice to use unprefixed `vizType` as classnames for chart // container. It may cause css conflicts as in the case of legacy table chart. // When migrating charts, we should gradually add a `superset-chart-` prefix @@ -255,11 +257,11 @@ class ChartRenderer extends React.Component { annotationData={annotationData} datasource={datasource} initialValues={initialValues} - formData={formData} + formData={currentFormData} ownState={ownState} filterState={filterState} hooks={this.hooks} - behaviors={[Behavior.INTERACTIVE_CHART]} + behaviors={behaviors} queriesData={queriesResponse} onRenderSuccess={this.handleRenderSuccess} onRenderFailure={this.handleRenderFailure} diff --git a/superset-frontend/src/components/Chart/ChartRenderer.test.jsx b/superset-frontend/src/components/Chart/ChartRenderer.test.jsx index 7e3a455631ff..f3ce0415175f 100644 --- a/superset-frontend/src/components/Chart/ChartRenderer.test.jsx +++ b/superset-frontend/src/components/Chart/ChartRenderer.test.jsx @@ -25,22 +25,25 @@ import ChartRenderer from 'src/components/Chart/ChartRenderer'; const requiredProps = { chartId: 1, datasource: {}, - formData: {}, - vizType: 'foo', + formData: { testControl: 'foo' }, + latestQueryFormData: { + testControl: 'bar', + }, + vizType: 'table', }; describe('ChartRenderer', () => { it('should render SuperChart', () => { const wrapper = shallow( - , + , ); expect(wrapper.find(SuperChart)).toExist(); }); - it('should not render SuperChart when refreshOverlayVisible is true', () => { - const wrapper = shallow( - , - ); - expect(wrapper.find(SuperChart)).not.toExist(); + it('should use latestQueryFormData instead of formData when chartIsStale is true', () => { + const wrapper = shallow(); + expect(wrapper.find(SuperChart).prop('formData')).toEqual({ + testControl: 'bar', + }); }); }); diff --git a/superset-frontend/src/explore/components/ControlPanelAlert.tsx b/superset-frontend/src/explore/components/ControlPanelAlert.tsx deleted file mode 100644 index 142e074b559e..000000000000 --- a/superset-frontend/src/explore/components/ControlPanelAlert.tsx +++ /dev/null @@ -1,98 +0,0 @@ -/** - * 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. - */ - -import React from 'react'; -import { styled } from '@superset-ui/core'; -import Button from 'src/components/Button'; - -interface ControlPanelAlertProps { - title: string; - bodyText: string; - primaryButtonAction: (e: React.MouseEvent) => void; - secondaryButtonAction?: (e: React.MouseEvent) => void; - primaryButtonText: string; - secondaryButtonText?: string; - type: 'info' | 'warning'; -} - -const AlertContainer = styled.div` - margin: ${({ theme }) => theme.gridUnit * 4}px; - padding: ${({ theme }) => theme.gridUnit * 4}px; - - border: ${({ theme }) => `1px solid ${theme.colors.info.base}`}; - background-color: ${({ theme }) => theme.colors.info.light2}; - border-radius: 2px; - - color: ${({ theme }) => theme.colors.info.dark2}; - font-size: ${({ theme }) => theme.typography.sizes.s}; - - &.alert-type-warning { - border-color: ${({ theme }) => theme.colors.alert.base}; - background-color: ${({ theme }) => theme.colors.alert.light2}; - - p { - color: ${({ theme }) => theme.colors.alert.dark2}; - } - } -`; - -const ButtonContainer = styled.div` - display: flex; - justify-content: flex-end; - button { - line-height: 1; - } -`; - -const Title = styled.p` - font-weight: ${({ theme }) => theme.typography.weights.bold}; -`; - -export const ControlPanelAlert = ({ - title, - bodyText, - primaryButtonAction, - secondaryButtonAction, - primaryButtonText, - secondaryButtonText, - type = 'info', -}: ControlPanelAlertProps) => ( - - {title} -

{bodyText}

- - {secondaryButtonAction && secondaryButtonText && ( - - )} - - - -); diff --git a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx index e20e8574e35b..d4ef2690276a 100644 --- a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx +++ b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx @@ -60,7 +60,7 @@ import { Tooltip } from 'src/components/Tooltip'; import ControlRow from './ControlRow'; import Control from './Control'; -import { ControlPanelAlert } from './ControlPanelAlert'; +import { ExploreAlert } from './ExploreAlert'; import { RunQueryButton } from './RunQueryButton'; export type ControlPanelsContainerProps = { @@ -92,6 +92,7 @@ const actionButtonsContainerStyles = (theme: SupersetTheme) => css` flex-direction: column; align-items: center; padding: ${theme.gridUnit * 4}px; + z-index: 999; background: linear-gradient( transparent, ${theme.colors.grayscale.light5} ${theme.opacity.mediumLight} @@ -443,7 +444,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => { const DatasourceAlert = useCallback( () => hasControlsTransferred ? ( - { type="info" /> ) : ( - void; + secondaryButtonAction?: (e: React.MouseEvent) => void; + primaryButtonText?: string; + secondaryButtonText?: string; + type: 'info' | 'warning'; + className?: string; +} + +const AlertContainer = styled.div` + ${({ theme }) => css` + margin: ${theme.gridUnit * 4}px; + padding: ${theme.gridUnit * 4}px; + + border: 1px solid ${theme.colors.info.base}; + background-color: ${theme.colors.info.light2}; + border-radius: 2px; + + color: ${theme.colors.info.dark2}; + font-size: ${theme.typography.sizes.m}px; + + p { + margin-bottom: ${theme.gridUnit}px; + } + + & a, + & span[role='button'] { + color: inherit; + text-decoration: underline; + &:hover { + color: ${theme.colors.info.dark1}; + } + } + + &.alert-type-warning { + border-color: ${theme.colors.alert.base}; + background-color: ${theme.colors.alert.light2}; + + p { + color: ${theme.colors.alert.dark2}; + } + + & a:hover, + & span[role='button']:hover { + color: ${theme.colors.alert.dark1}; + } + } + `} +`; + +const ButtonContainer = styled.div` + display: flex; + justify-content: flex-end; + button { + line-height: 1; + } +`; + +const Title = styled.p` + font-weight: ${({ theme }) => theme.typography.weights.bold}; +`; + +export const ExploreAlert = forwardRef( + ( + { + title, + bodyText, + primaryButtonAction, + secondaryButtonAction, + primaryButtonText, + secondaryButtonText, + type = 'info', + className = '', + }: ControlPanelAlertProps, + ref: RefObject, + ) => ( + + {title} +

{bodyText}

+ {primaryButtonText && primaryButtonAction && ( + + {secondaryButtonAction && secondaryButtonText && ( + + )} + + + )} + + ), +); diff --git a/superset-frontend/src/explore/components/ExploreChartPanel.jsx b/superset-frontend/src/explore/components/ExploreChartPanel.jsx index 275b6fc40169..082d987b3cf4 100644 --- a/superset-frontend/src/explore/components/ExploreChartPanel.jsx +++ b/superset-frontend/src/explore/components/ExploreChartPanel.jsx @@ -19,7 +19,14 @@ import React, { useCallback, useEffect, useMemo, useState } from 'react'; import PropTypes from 'prop-types'; import Split from 'react-split'; -import { css, styled, SupersetClient, useTheme } from '@superset-ui/core'; +import { + css, + ensureIsArray, + styled, + SupersetClient, + t, + useTheme, +} from '@superset-ui/core'; import { useResizeDetector } from 'react-resize-detector'; import { chartPropShape } from 'src/dashboard/util/propShapes'; import ChartContainer from 'src/components/Chart/ChartContainer'; @@ -31,6 +38,8 @@ import { import { DataTablesPane } from './DataTablesPane'; import { buildV1ChartDataPayload } from '../exploreUtils'; import { ChartPills } from './ChartPills'; +import { ExploreAlert } from './ExploreAlert'; +import { getChartRequiredFieldsMissingMessage } from '../../utils/getChartRequiredFieldsMissingMessage'; const propTypes = { actions: PropTypes.object.isRequired, @@ -51,7 +60,7 @@ const propTypes = { standalone: PropTypes.number, force: PropTypes.bool, timeout: PropTypes.number, - refreshOverlayVisible: PropTypes.bool, + chartIsStale: PropTypes.bool, chart: chartPropShape, errorMessage: PropTypes.node, triggerRender: PropTypes.bool, @@ -115,10 +124,11 @@ const ExploreChartPanel = ({ errorMessage, form_data: formData, onQuery, - refreshOverlayVisible, actions, timeout, standalone, + chartIsStale, + chartAlert, }) => { const theme = useTheme(); const gutterMargin = theme.gridUnit * GUTTER_SIZE_FACTOR; @@ -134,6 +144,13 @@ const ExploreChartPanel = ({ const [splitSizes, setSplitSizes] = useState( getItem(LocalStorageKeys.chart_split_sizes, INITIAL_SIZES), ); + + const showAlertBanner = + !chartAlert && + chartIsStale && + chart.chartStatus !== 'failed' && + ensureIsArray(chart.queriesResponse).length > 0; + const updateQueryContext = useCallback( async function fetchChartData() { if (slice && slice.query_context === null) { @@ -167,11 +184,11 @@ const ExploreChartPanel = ({ setItem(LocalStorageKeys.chart_split_sizes, splitSizes); }, [splitSizes]); - const onDragEnd = sizes => { + const onDragEnd = useCallback(sizes => { setSplitSizes(sizes); - }; + }, []); - const refreshCachedQuery = () => { + const refreshCachedQuery = useCallback(() => { actions.postChartFormData( formData, true, @@ -180,7 +197,7 @@ const ExploreChartPanel = ({ undefined, ownState, ); - }; + }, [actions, chart.id, formData, ownState, timeout]); const onCollapseChange = useCallback(isOpen => { let splitSizes; @@ -219,9 +236,10 @@ const ExploreChartPanel = ({ datasource={datasource} errorMessage={errorMessage} formData={formData} + latestQueryFormData={chart.latestQueryFormData} onQuery={onQuery} queriesResponse={chart.queriesResponse} - refreshOverlayVisible={refreshOverlayVisible} + chartIsStale={chartIsStale} setControlValue={actions.setControlValue} timeout={timeout} triggerQuery={chart.triggerQuery} @@ -237,8 +255,10 @@ const ExploreChartPanel = ({ chart.chartStackTrace, chart.chartStatus, chart.id, + chart.latestQueryFormData, chart.queriesResponse, chart.triggerQuery, + chartIsStale, chartPanelHeight, chartPanelRef, chartPanelWidth, @@ -248,7 +268,6 @@ const ExploreChartPanel = ({ formData, onQuery, ownState, - refreshOverlayVisible, timeout, triggerRender, vizType, @@ -264,6 +283,34 @@ const ExploreChartPanel = ({ flex-direction: column; `} > + {showAlertBanner && ( + + {t( + 'You updated the values in the control panel, but the chart was not updated automatically. Run the query by clicking on the "Update chart" button or', + )}{' '} + + {t('click here')} + + . + + ) + } + type="warning" + css={theme => css` + margin: 0 0 ${theme.gridUnit * 4}px 0; + `} + /> + )} ), - [chartPanelRef, renderChart], + [ + showAlertBanner, + errorMessage, + onQuery, + chart.queriesResponse, + chart.chartStatus, + chart.chartUpdateStartTime, + chart.chartUpdateEndTime, + refreshCachedQuery, + formData?.row_limit, + renderChart, + ], ); const standaloneChartBody = useMemo(() => renderChart(), [renderChart]); @@ -294,6 +352,13 @@ const ExploreChartPanel = ({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [chart.latestQueryFormData]); + const elementStyle = useCallback( + (dimension, elementSize, gutterSize) => ({ + [dimension]: `calc(${elementSize}% - ${gutterSize + gutterMargin}px)`, + }), + [gutterMargin], + ); + if (standalone) { // dom manipulation hack to get rid of the boostrap theme's body background const standaloneClass = 'background-transparent'; @@ -304,10 +369,6 @@ const ExploreChartPanel = ({ return standaloneChartBody; } - const elementStyle = (dimension, elementSize, gutterSize) => ({ - [dimension]: `calc(${elementSize}% - ${gutterSize + gutterMargin}px)`, - }); - return ( {vizType === 'filter_box' ? ( diff --git a/superset-frontend/src/explore/components/ExploreChartPanel.test.jsx b/superset-frontend/src/explore/components/ExploreChartPanel.test.jsx index c50a605a40aa..a779773052e6 100644 --- a/superset-frontend/src/explore/components/ExploreChartPanel.test.jsx +++ b/superset-frontend/src/explore/components/ExploreChartPanel.test.jsx @@ -17,23 +17,70 @@ * under the License. */ import React from 'react'; - +import { render, screen } from 'spec/helpers/testing-library'; import ChartContainer from 'src/explore/components/ExploreChartPanel'; -describe('ChartContainer', () => { - const mockProps = { - sliceName: 'Trend Line', - vizType: 'line', - height: '500px', - actions: {}, - can_overwrite: false, - can_download: false, - containerId: 'foo', - width: '50px', - isStarred: false, - }; +const createProps = (overrides = {}) => ({ + sliceName: 'Trend Line', + vizType: 'line', + height: '500px', + actions: {}, + can_overwrite: false, + can_download: false, + containerId: 'foo', + width: '500px', + isStarred: false, + chartIsStale: false, + chart: {}, + form_data: {}, + ...overrides, +}); +describe('ChartContainer', () => { it('renders when vizType is line', () => { - expect(React.isValidElement()).toBe(true); + const props = createProps(); + expect(React.isValidElement()).toBe(true); + }); + + it('renders with alert banner', () => { + const props = createProps({ + chartIsStale: true, + chart: { chartStatus: 'rendered', queriesResponse: [{}] }, + }); + render(, { useRedux: true }); + expect(screen.getByText('Your chart is not up to date')).toBeVisible(); + }); + + it('doesnt render alert banner when no changes in control panel were made (chart is not stale)', () => { + const props = createProps({ + chartIsStale: false, + }); + render(, { useRedux: true }); + expect( + screen.queryByText('Your chart is not up to date'), + ).not.toBeInTheDocument(); + }); + + it('doesnt render alert banner when chart not created yet (no queries response)', () => { + const props = createProps({ + chartIsStale: true, + chart: { queriesResponse: [] }, + }); + render(, { useRedux: true }); + expect( + screen.queryByText('Your chart is not up to date'), + ).not.toBeInTheDocument(); + }); + + it('renders prompt to fill required controls when required control removed', () => { + const props = createProps({ + chartIsStale: true, + chart: { chartStatus: 'rendered', queriesResponse: [{}] }, + errorMessage: 'error', + }); + render(, { useRedux: true }); + expect( + screen.getByText('Required control values have been removed'), + ).toBeVisible(); }); }); diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx index a240578c49fc..7743997a3552 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx @@ -27,7 +27,10 @@ import ExploreViewContainer from '.'; const reduxState = { explore: { common: { conf: { SUPERSET_WEBSERVER_TIMEOUT: 60 } }, - controls: { datasource: { value: '1__table' } }, + controls: { + datasource: { value: '1__table' }, + viz_type: { value: 'table' }, + }, datasource: { id: 1, type: 'table', diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx index a9ec1f26740c..da18dcc4ff5c 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx @@ -21,8 +21,8 @@ import React, { useCallback, useEffect, useMemo, useState } from 'react'; import PropTypes from 'prop-types'; import { bindActionCreators } from 'redux'; import { connect } from 'react-redux'; -import { css, logging, styled, t, useTheme } from '@superset-ui/core'; -import { debounce } from 'lodash'; +import { styled, t, css, useTheme, logging } from '@superset-ui/core'; +import { debounce, pick } from 'lodash'; import { Resizable } from 're-resizable'; import { useChangeEffect } from 'src/hooks/useChangeEffect'; import { usePluginContext } from 'src/components/DynamicPlugins'; @@ -33,8 +33,8 @@ import { useComponentDidMount } from 'src/hooks/useComponentDidMount'; import Icons from 'src/components/Icons'; import { getItem, - LocalStorageKeys, setItem, + LocalStorageKeys, } from 'src/utils/localStorageHelpers'; import { RESERVED_CHART_URL_PARAMS, URL_PARAMS } from 'src/constants'; import { getUrlParam } from 'src/utils/urlUtils'; @@ -56,8 +56,8 @@ import * as exploreActions from '../../actions/exploreActions'; import * as saveModalActions from '../../actions/saveModalActions'; import * as logActions from '../../../logger/actions'; import { - LOG_ACTIONS_CHANGE_EXPLORE_CONTROLS, LOG_ACTIONS_MOUNT_EXPLORER, + LOG_ACTIONS_CHANGE_EXPLORE_CONTROLS, } from '../../../logger/LogUtils'; import ConnectedExploreChartHeader from '../ExploreChartHeader'; @@ -381,18 +381,33 @@ function ExploreViewContainer(props) { } }, []); - const reRenderChart = () => { - props.actions.updateQueryFormData( - getFormDataFromControls(props.controls), + const reRenderChart = useCallback( + controlsChanged => { + const newQueryFormData = controlsChanged + ? { + ...props.chart.latestQueryFormData, + ...getFormDataFromControls(pick(props.controls, controlsChanged)), + } + : getFormDataFromControls(props.controls); + props.actions.updateQueryFormData(newQueryFormData, props.chart.id); + props.actions.renderTriggered(new Date().getTime(), props.chart.id); + addHistory(); + }, + [ + addHistory, + props.actions, props.chart.id, - ); - props.actions.renderTriggered(new Date().getTime(), props.chart.id); - addHistory(); - }; + props.chart.latestQueryFormData, + props.controls, + ], + ); // effect to run when controls change useEffect(() => { - if (previousControls) { + if ( + previousControls && + props.chart.latestQueryFormData.viz_type === props.controls.viz_type.value + ) { if ( props.controls.datasource && (previousControls.datasource == null || @@ -412,11 +427,11 @@ function ExploreViewContainer(props) { ); // this should also be handled by the actions that are actually changing the controls - const hasDisplayControlChanged = changedControlKeys.some( + const displayControlsChanged = changedControlKeys.filter( key => props.controls[key].renderTrigger, ); - if (hasDisplayControlChanged) { - reRenderChart(); + if (displayControlsChanged.length > 0) { + reRenderChart(displayControlsChanged); } } }, [props.controls, props.ownState]); @@ -493,7 +508,7 @@ function ExploreViewContainer(props) { ); diff --git a/superset-frontend/src/explore/controlUtils/getFormDataFromControls.ts b/superset-frontend/src/explore/controlUtils/getFormDataFromControls.ts index f5ffa523c359..ba9419da1873 100644 --- a/superset-frontend/src/explore/controlUtils/getFormDataFromControls.ts +++ b/superset-frontend/src/explore/controlUtils/getFormDataFromControls.ts @@ -22,13 +22,10 @@ import { ControlStateMapping } from '@superset-ui/chart-controls'; export function getFormDataFromControls( controlsState: ControlStateMapping, ): QueryFormData { - const formData: QueryFormData = { - viz_type: 'table', - datasource: '', - }; + const formData = {}; Object.keys(controlsState).forEach(controlName => { const control = controlsState[controlName]; formData[controlName] = control.value; }); - return formData; + return formData as QueryFormData; } diff --git a/superset-frontend/src/utils/getChartRequiredFieldsMissingMessage.ts b/superset-frontend/src/utils/getChartRequiredFieldsMissingMessage.ts new file mode 100644 index 000000000000..ac11e8503dc2 --- /dev/null +++ b/superset-frontend/src/utils/getChartRequiredFieldsMissingMessage.ts @@ -0,0 +1,26 @@ +/** + * 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. + */ + +import { t } from '@superset-ui/core'; + +export const getChartRequiredFieldsMissingMessage = (isCreating: boolean) => + t( + 'Select values in highlighted field(s) in the control panel. Then run the query by clicking on the %s button.', + isCreating ? '"Create chart"' : '"Update chart"', + );