Skip to content

Commit

Permalink
feat(explore): Replace overlay with alert banner when chart controls …
Browse files Browse the repository at this point in the history
…change (apache#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
  • Loading branch information
kgabryje authored and philipher29 committed Jun 9, 2022
1 parent 0e6b85f commit 6b62d32
Show file tree
Hide file tree
Showing 12 changed files with 375 additions and 217 deletions.
64 changes: 19 additions & 45 deletions superset-frontend/src/components/Chart/Chart.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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"
/>
);
Expand All @@ -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"
/>
);
Expand All @@ -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>
Expand Down
28 changes: 15 additions & 13 deletions superset-frontend/src/components/Chart/ChartRenderer.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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;
}

Expand All @@ -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
Expand Down Expand Up @@ -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}
Expand Down
19 changes: 11 additions & 8 deletions superset-frontend/src/components/Chart/ChartRenderer.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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',
});
});
});
98 changes: 0 additions & 98 deletions superset-frontend/src/explore/components/ControlPanelAlert.tsx

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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}
Expand Down Expand Up @@ -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.",
Expand All @@ -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.',
Expand Down
Loading

0 comments on commit 6b62d32

Please sign in to comment.