From dd2eb1babae64b1140ad8cc35580c88ccb02edbf Mon Sep 17 00:00:00 2001 From: geido Date: Thu, 11 Nov 2021 16:01:33 +0000 Subject: [PATCH 1/6] Add alert for custom label colors --- .../components/ColorSchemeControlWrapper.jsx | 6 +- .../components/PropertiesModal/index.jsx | 30 ++++++-- .../ColorSchemeControl.test.tsx | 68 +++++++++++++++++++ .../index.jsx} | 43 ++++++++++-- 4 files changed, 136 insertions(+), 11 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} (78%) diff --git a/superset-frontend/src/dashboard/components/ColorSchemeControlWrapper.jsx b/superset-frontend/src/dashboard/components/ColorSchemeControlWrapper.jsx index 18af6f5a809de..4a7839fdbb2c5 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 9597ccfccd3c6..8acff9867dd2c 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 78% rename from superset-frontend/src/explore/components/controls/ColorSchemeControl.jsx rename to superset-frontend/src/explore/components/controls/ColorSchemeControl/index.jsx index 20be45db038b0..3b125b663b4e8 100644 --- a/superset-frontend/src/explore/components/controls/ColorSchemeControl.jsx +++ b/superset-frontend/src/explore/components/controls/ColorSchemeControl/index.jsx @@ -21,12 +21,14 @@ 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, description: PropTypes.string, - label: PropTypes.string.isRequired, + label: PropTypes.string, labelMargin: PropTypes.number, name: PropTypes.string.isRequired, onChange: PropTypes.func, @@ -43,16 +45,23 @@ 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); } onChange(value) { @@ -105,8 +114,29 @@ export default class ColorSchemeControl extends React.PureComponent { ); } + renderLabel() { + const { hasCustomLabelColors, label } = this.props; + + if (hasCustomLabelColors) { + return ( + <> + {label}{' '} + + + + + ); + } + return label; + } + render() { - const { schemes, choices } = this.props; + const { choices, schemes } = this.props; // save parsed schemes for later this.schemes = isFunction(schemes) ? schemes() : schemes; @@ -137,7 +167,10 @@ export default class ColorSchemeControl extends React.PureComponent { value: currentScheme, }; return ( - } + {...selectProps} + /> ); } } From a2a16a6193ae2cfc7eced25209450f26187da3fe Mon Sep 17 00:00:00 2001 From: geido Date: Thu, 11 Nov 2021 16:23:18 +0000 Subject: [PATCH 2/6] Fix duplicate linear scheme --- .../controls/ColorSchemeControl/index.jsx | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/ColorSchemeControl/index.jsx b/superset-frontend/src/explore/components/controls/ColorSchemeControl/index.jsx index 3b125b663b4e8..d0d29ae15e93a 100644 --- a/superset-frontend/src/explore/components/controls/ColorSchemeControl/index.jsx +++ b/superset-frontend/src/explore/components/controls/ColorSchemeControl/index.jsx @@ -139,16 +139,20 @@ export default class ColorSchemeControl extends React.PureComponent { const { choices, schemes } = this.props; // save parsed schemes for later this.schemes = isFunction(schemes) ? schemes() : schemes; - - const allColorOptions = (isFunction(choices) ? choices() : choices).filter( - o => o[0] !== 'SUPERSET_DEFAULT', - ); - const options = allColorOptions.map(([value]) => ({ - value, - label: this.schemes?.[value]?.label || value, + 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; + }); + const options = filteredColorOptions.map(([value]) => ({ customLabel: this.renderOption(value), + label: this.schemes?.[value]?.label || value, + value, })); - let currentScheme = this.props.value || (this.props.default !== undefined ? this.props.default : undefined); From 8020e0e937a6c2a8501e2dbd2e6b1d158c1efba4 Mon Sep 17 00:00:00 2001 From: geido Date: Fri, 12 Nov 2021 11:15:44 +0000 Subject: [PATCH 3/6] Implement dashboard alert --- .../controls/ColorSchemeControl/index.jsx | 95 ++++++++++++------- 1 file changed, 61 insertions(+), 34 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/ColorSchemeControl/index.jsx b/superset-frontend/src/explore/components/controls/ColorSchemeControl/index.jsx index d0d29ae15e93a..239e9eadbe662 100644 --- a/superset-frontend/src/explore/components/controls/ColorSchemeControl/index.jsx +++ b/superset-frontend/src/explore/components/controls/ColorSchemeControl/index.jsx @@ -27,6 +27,7 @@ import ControlHeader from 'src/explore/components/ControlHeader'; const propTypes = { hasCustomLabelColors: PropTypes.bool, + dashboardId: PropTypes.number, description: PropTypes.string, label: PropTypes.string, labelMargin: PropTypes.number, @@ -62,6 +63,10 @@ export default class ColorSchemeControl extends React.PureComponent { 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) { @@ -81,7 +86,7 @@ export default class ColorSchemeControl extends React.PureComponent { } return ( - +
    ))}
- + ); } renderLabel() { - const { hasCustomLabelColors, label } = this.props; + const { dashboardId, hasCustomLabelColors, label } = this.props; - if (hasCustomLabelColors) { + 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}{' '} - + @@ -136,40 +142,61 @@ export default class ColorSchemeControl extends React.PureComponent { } render() { - const { choices, schemes } = this.props; - // save parsed schemes for later - 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; - }); - const options = filteredColorOptions.map(([value]) => ({ - customLabel: this.renderOption(value), - label: this.schemes?.[value]?.label || value, - value, - })); - let currentScheme = - this.props.value || - (this.props.default !== undefined ? this.props.default : undefined); - - if (currentScheme === 'SUPERSET_DEFAULT') { - currentScheme = this.schemes?.SUPERSET_DEFAULT?.id; + const { choices, dashboardId, schemes } = this.props; + let options = dashboardId + ? [ + { + value: 'dashboard', + label: 'dashboard', + customLabel: ( + + {t('Dashboard scheme')} + + ), + }, + ] + : []; + let currentScheme = dashboardId ? 'dashboard' : undefined; + + // 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; + }); + + options = filteredColorOptions.map(([value]) => ({ + customLabel: this.renderOption(value), + label: this.schemes?.[value]?.label || value, + value, + })); + + currentScheme = + this.props.value || + (this.props.default !== undefined ? this.props.default : undefined); + + 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 (