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
apache · Apr 19, 2022 · 6f4480a · 6f4480a
1 parent 594523e
commit 6f4480a
Show file tree

Hide file tree

Showing 12 changed files with 372 additions and 214 deletions.
diff --git 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 (
         <EmptyStateBig
           title={t('Add required control values to preview chart')}
-          description={description}
+          description={getChartRequiredFieldsMissingMessage(true)}
           image="chart.svg"
         />
       );
@@ -291,15 +268,24 @@ class Chart extends React.PureComponent {
     if (
       !isLoading &&
       !chartAlert &&
-      isFaded &&
+      !errorMessage &&
+      chartIsStale &&
       ensureIsArray(queriesResponse).length === 0
     ) {
       return (
         <EmptyStateBig
           title={t('Your chart is ready to go!')}
-          description={t(
-            'Click on "Create chart" button in the control panel on the left to preview a visualization',
-          )}
+          description={
+            <span>
+              {t(
+                'Click on "Create chart" button in the control panel on the left to preview a visualization or',
+              )}{' '}
+              <span role="button" tabIndex={0} onClick={this.props.onQuery}>
+                {t('click here')}
+              </span>
+              .
+            </span>
+          }
           image="chart.svg"
         />
       );
@@ -317,25 +303,13 @@ class Chart extends React.PureComponent {
           height={height}
           width={width}
         >
-          <div
-            className={`slice_container ${isFaded ? ' faded' : ''}`}
-            data-test="slice-container"
-          >
+          <div className="slice_container" data-test="slice-container">
             <ChartRenderer
               {...this.props}
               source={this.props.dashboardId ? 'dashboard' : 'explore'}
               data-test={this.props.vizType}
             />
           </div>
-
-          {!isLoading && !chartAlert && isFaded && (
-            <RefreshOverlayWrapper>
-              <Button onClick={onQuery} buttonStyle="primary">
-                {t('Run query')}
-              </Button>
-            </RefreshOverlayWrapper>
-          )}
-
           {isLoading && !isDeactivatedViz && <Loading />}
         </Styles>
       </ErrorBoundary>

diff --git 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
@@ -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(
-      <ChartRenderer {...requiredProps} refreshOverlayVisible={false} />,
+      <ChartRenderer {...requiredProps} chartIsStale={false} />,
     );
     expect(wrapper.find(SuperChart)).toExist();
   });
 
-  it('should not render SuperChart when refreshOverlayVisible is true', () => {
-    const wrapper = shallow(
-      <ChartRenderer {...requiredProps} refreshOverlayVisible />,
-    );
-    expect(wrapper.find(SuperChart)).not.toExist();
+  it('should use latestQueryFormData instead of formData when chartIsStale is true', () => {
+    const wrapper = shallow(<ChartRenderer {...requiredProps} chartIsStale />);
+    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
diff --git 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 ? (
-        <ControlPanelAlert
+        <ExploreAlert
           title={t('Keep control settings?')}
           bodyText={t(
             "You've changed datasets. Any controls with data (columns, metrics) that match this new dataset have been retained.",
@@ -455,7 +456,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
           type="info"
         />
       ) : (
-        <ControlPanelAlert
+        <ExploreAlert
           title={t('No form settings were maintained')}
           bodyText={t(
             'We were unable to carry over any controls when switching to this new dataset.',