From c2d8b0e72f5146bca0b7ef59f8fe28972c7664e4 Mon Sep 17 00:00:00 2001 From: Geido <60598000+geido@users.noreply.github.com> Date: Tue, 16 Nov 2021 17:46:39 +0200 Subject: [PATCH] chore: Notify user of custom label colors and related Dashboard color scheme (#17422) * Add alert for custom label colors * Fix duplicate linear scheme * Implement dashboard alert * Remove trailing space * Update Cypress * Simplify check --- .../explore/visualizations/line.test.ts | 2 +- .../components/ColorSchemeControlWrapper.jsx | 6 +- .../components/PropertiesModal/index.jsx | 30 ++++- .../ColorSchemeControl.test.tsx | 68 +++++++++++ .../index.jsx} | 108 ++++++++++++++---- 5 files changed, 184 insertions(+), 30 deletions(-) create mode 100644 superset-frontend/src/explore/components/controls/ColorSchemeControl/ColorSchemeControl.test.tsx rename superset-frontend/src/explore/components/controls/{ColorSchemeControl.jsx => ColorSchemeControl/index.jsx} (53%) diff --git a/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts b/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts index 676c3b6d53..019bad7096 100644 --- a/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts @@ -71,7 +71,7 @@ describe('Visualization > Line', () => { .focus() .type('bnbColors{enter}'); cy.get( - '.Control[data-test="color_scheme"] .ant-select-selection-item > ul[data-test="bnbColors"]', + '.Control[data-test="color_scheme"] .ant-select-selection-item ul[data-test="bnbColors"]', ).should('exist'); }); diff --git a/superset-frontend/src/dashboard/components/ColorSchemeControlWrapper.jsx b/superset-frontend/src/dashboard/components/ColorSchemeControlWrapper.jsx index 18af6f5a80..4a7839fdbb 100644 --- a/superset-frontend/src/dashboard/components/ColorSchemeControlWrapper.jsx +++ b/superset-frontend/src/dashboard/components/ColorSchemeControlWrapper.jsx @@ -27,9 +27,11 @@ const propTypes = { onChange: PropTypes.func, labelMargin: PropTypes.number, colorScheme: PropTypes.string, + hasCustomLabelColors: PropTypes.bool, }; const defaultProps = { + hasCustomLabelColors: false, colorScheme: undefined, onChange: () => {}, }; @@ -48,13 +50,12 @@ class ColorSchemeControlWrapper extends React.PureComponent { } render() { - const { colorScheme, labelMargin = 0 } = this.props; + const { colorScheme, labelMargin = 0, hasCustomLabelColors } = this.props; return ( ); } diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/index.jsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.jsx index 9597ccfccd..8acff9867d 100644 --- a/superset-frontend/src/dashboard/components/PropertiesModal/index.jsx +++ b/superset-frontend/src/dashboard/components/PropertiesModal/index.jsx @@ -132,6 +132,7 @@ class PropertiesModal extends React.PureComponent { this.onColorSchemeChange = this.onColorSchemeChange.bind(this); this.getRowsWithRoles = this.getRowsWithRoles.bind(this); this.getRowsWithoutRoles = this.getRowsWithoutRoles.bind(this); + this.getJsonMetadata = this.getJsonMetadata.bind(this); } componentDidMount() { @@ -139,13 +140,22 @@ class PropertiesModal extends React.PureComponent { JsonEditor.preload(); } + getJsonMetadata() { + const { json_metadata: jsonMetadata } = this.state.values; + try { + const jsonMetadataObj = jsonMetadata?.length + ? JSON.parse(jsonMetadata) + : {}; + return jsonMetadataObj; + } catch (_) { + return {}; + } + } + onColorSchemeChange(colorScheme, { updateMetadata = true } = {}) { // check that color_scheme is valid const colorChoices = getCategoricalSchemeRegistry().keys(); - const { json_metadata: jsonMetadata } = this.state.values; - const jsonMetadataObj = jsonMetadata?.length - ? JSON.parse(jsonMetadata) - : {}; + const jsonMetadataObj = this.getJsonMetadata(); // only fire if the color_scheme is present and invalid if (colorScheme && !colorChoices.includes(colorScheme)) { @@ -318,6 +328,11 @@ class PropertiesModal extends React.PureComponent { getRowsWithoutRoles() { const { values, isDashboardLoaded } = this.state; + const jsonMetadataObj = this.getJsonMetadata(); + const hasCustomLabelColors = !!Object.keys( + jsonMetadataObj?.label_colors || {}, + ).length; + return ( @@ -343,6 +358,7 @@ class PropertiesModal extends React.PureComponent {

{t('Colors')}

@@ -404,6 +425,7 @@ class PropertiesModal extends React.PureComponent { null, + isLinear: false, +}; + +const setup = (overrides?: Record) => + render(); + +test('should render', async () => { + const { container } = setup(); + await waitFor(() => expect(container).toBeVisible()); +}); + +test('should display a label', async () => { + setup(); + expect(await screen.findByText('Color scheme')).toBeTruthy(); +}); + +test('should not display an alert icon if hasCustomLabelColors=false', async () => { + setup(); + await waitFor(() => { + expect( + screen.queryByRole('img', { name: 'alert-solid' }), + ).not.toBeInTheDocument(); + }); +}); + +test('should display an alert icon if hasCustomLabelColors=true', async () => { + const hasCustomLabelColorsProps = { + ...defaultProps, + hasCustomLabelColors: true, + }; + setup(hasCustomLabelColorsProps); + await waitFor(() => { + expect( + screen.getByRole('img', { name: 'alert-solid' }), + ).toBeInTheDocument(); + }); +}); diff --git a/superset-frontend/src/explore/components/controls/ColorSchemeControl.jsx b/superset-frontend/src/explore/components/controls/ColorSchemeControl/index.jsx similarity index 53% rename from superset-frontend/src/explore/components/controls/ColorSchemeControl.jsx rename to superset-frontend/src/explore/components/controls/ColorSchemeControl/index.jsx index 20be45db03..f0ccc12398 100644 --- a/superset-frontend/src/explore/components/controls/ColorSchemeControl.jsx +++ b/superset-frontend/src/explore/components/controls/ColorSchemeControl/index.jsx @@ -21,12 +21,15 @@ import PropTypes from 'prop-types'; import { isFunction } from 'lodash'; import { Select } from 'src/components'; import { Tooltip } from 'src/components/Tooltip'; -import { t } from '@superset-ui/core'; -import ControlHeader from '../ControlHeader'; +import { styled, t } from '@superset-ui/core'; +import Icons from 'src/components/Icons'; +import ControlHeader from 'src/explore/components/ControlHeader'; const propTypes = { + hasCustomLabelColors: PropTypes.bool, + dashboardId: PropTypes.number, description: PropTypes.string, - label: PropTypes.string.isRequired, + label: PropTypes.string, labelMargin: PropTypes.number, name: PropTypes.string.isRequired, onChange: PropTypes.func, @@ -43,16 +46,27 @@ const propTypes = { const defaultProps = { choices: [], + hasCustomLabelColors: false, + label: t('Color scheme'), schemes: {}, clearable: false, onChange: () => {}, }; +const StyledAlert = styled(Icons.AlertSolid)` + color: ${({ theme }) => theme.colors.alert.base}; +`; + export default class ColorSchemeControl extends React.PureComponent { constructor(props) { super(props); this.onChange = this.onChange.bind(this); this.renderOption = this.renderOption.bind(this); + this.renderLabel = this.renderLabel.bind(this); + this.dashboardColorSchemeAlert = t( + `The color scheme is determined by the related dashboard. + Edit the color scheme in the dashboard properties.`, + ); } onChange(value) { @@ -72,7 +86,7 @@ export default class ColorSchemeControl extends React.PureComponent { } return ( - +
    ))}
- + ); } + renderLabel() { + const { dashboardId, hasCustomLabelColors, label } = this.props; + + if (hasCustomLabelColors || dashboardId) { + const alertTitle = hasCustomLabelColors + ? t( + `This color scheme is being overriden by custom label colors. + Check the JSON metadata in the Advanced settings`, + ) + : this.dashboardColorSchemeAlert; + return ( + <> + {label}{' '} + + + + + ); + } + return label; + } + render() { - const { schemes, choices } = this.props; - // save parsed schemes for later - this.schemes = isFunction(schemes) ? schemes() : schemes; + const { choices, dashboardId, schemes } = this.props; + let options = dashboardId + ? [ + { + value: 'dashboard', + label: 'dashboard', + customLabel: ( + + {t('Dashboard scheme')} + + ), + }, + ] + : []; + let currentScheme = dashboardId ? 'dashboard' : undefined; - const allColorOptions = (isFunction(choices) ? choices() : choices).filter( - o => o[0] !== 'SUPERSET_DEFAULT', - ); - const options = allColorOptions.map(([value]) => ({ - value, - label: this.schemes?.[value]?.label || value, - customLabel: this.renderOption(value), - })); + // if related to a dashboard the scheme is dictated by the dashboard + if (!dashboardId) { + this.schemes = isFunction(schemes) ? schemes() : schemes; + const controlChoices = isFunction(choices) ? choices() : choices; + const allColorOptions = []; + const filteredColorOptions = controlChoices.filter(o => { + const option = o[0]; + const isValidColorOption = + option !== 'SUPERSET_DEFAULT' && !allColorOptions.includes(option); + allColorOptions.push(option); + return isValidColorOption; + }); - let currentScheme = - this.props.value || - (this.props.default !== undefined ? this.props.default : undefined); + options = filteredColorOptions.map(([value]) => ({ + customLabel: this.renderOption(value), + label: this.schemes?.[value]?.label || value, + value, + })); - if (currentScheme === 'SUPERSET_DEFAULT') { - currentScheme = this.schemes?.SUPERSET_DEFAULT?.id; + currentScheme = this.props.value || this.props.default; + + if (currentScheme === 'SUPERSET_DEFAULT') { + currentScheme = this.schemes?.SUPERSET_DEFAULT?.id; + } } const selectProps = { ariaLabel: t('Select color scheme'), allowClear: this.props.clearable, + disabled: !!dashboardId, name: `select-${this.props.name}`, onChange: this.onChange, options, - placeholder: `Select (${options.length})`, + placeholder: t('Select scheme'), value: currentScheme, }; + return ( - } + {...selectProps} + /> ); } }