sadpandajoe commented on code in PR #42910: URL: https://github.com/apache/superset/pull/42910#discussion_r4170141279
########## superset-frontend/src/explore/components/controls/IntervalColorsControl/legacyColors.ts: ########## @@ -0,0 +1,71 @@ +/** + * 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 { getCategoricalSchemeRegistry } from '@superset-ui/core'; + +export const parseIntervalBounds = (intervals?: string): number[] => + (intervals ?? '') + .split(',') + .map(part => part.trim()) + .filter(part => part !== '') + .map(Number) + .filter(bound => Number.isFinite(bound)); + +/** + * Resolves the legacy `interval_color_indices` control (comma-separated, + * 1-indexed positions into the chosen categorical color scheme) into real + * hex colors, positionally matched to `bounds`. Shared by the control panel + * (display-only fallback while a legacy chart is being edited) and by + * `handleDeprecatedControls` (which migrates the value into `interval_colors` + * on load, since `interval_color_indices` is no longer a registered control + * and would otherwise be silently dropped on the next save). + */ +export const resolveLegacyIntervalColors = ( + bounds: number[], + legacyIntervalColorIndices: string | undefined, + colorScheme: string | undefined, +): string[] => { + // strict: an explicitly-named but unregistered scheme should resolve to + // "no colors" here, not silently fall back to the registry's default + // scheme -- that fallback belongs to the display layer (index.tsx), not + // to this migration helper. + const schemeColors = + getCategoricalSchemeRegistry().get(colorScheme, true)?.colors ?? []; Review Comment: For an imported Gauge whose saved palette is no longer registered, the renderer still resolves legacy indices against the default palette, but this strict lookup leaves `interval_colors` unset; Explore then drops `interval_color_indices` on save and loses those picks. Could this preserve the renderer’s default-palette fallback, or retain the legacy value until it can be migrated? ########## superset-frontend/src/explore/store.ts: ########## @@ -89,6 +105,50 @@ export function handleDeprecatedControls(formData: FormData): void { formData.matrixify_mode_columns = 'disabled'; } } + + // #42910: migrate the legacy BigNumberPeriodOverPeriod + // `comparison_color_scheme` ('Green' | 'Red', where 'Red' reverses + // increase/decrease colors) into the `increase_color` / `decrease_color` + // ColorPickerControls that replaced it. `comparison_color_scheme` is no + // longer a registered control, so `getFormDataFromControls` drops it the + // next time the chart is saved -- without this migration, that silently + // discards a reversed-color choice the first time an old chart is resaved. + if ( + formData.viz_type === VizType.BigNumberPeriodOverPeriod && + formData.comparison_color_scheme && + formData.increase_color === undefined && + formData.decrease_color === undefined + ) { + const legacyReversed = + formData.comparison_color_scheme === ColorSchemeEnum.Red; + formData.increase_color = legacyReversed + ? ColorSchemeEnum.Red + : ColorSchemeEnum.Green; + formData.decrease_color = legacyReversed + ? ColorSchemeEnum.Green + : ColorSchemeEnum.Red; + } + + // #42910: migrate the legacy Gauge `interval_color_indices` + // (comma-separated, 1-indexed positions into `color_scheme`) into real hex + // colors stored by `interval_colors`, which replaced it. Same rationale as + // the comparison_color_scheme migration above -- `interval_color_indices` + // is no longer a registered control, so it's otherwise dropped on save. + if ( + formData.viz_type === VizType.Gauge && + formData.interval_color_indices && + (!formData.interval_colors || formData.interval_colors.length === 0) + ) { + const bounds = parseIntervalBounds(formData.intervals); + const resolved = resolveLegacyIntervalColors( Review Comment: Dashboard hydration runs this migration before applying the dashboard’s `color_scheme` override, so a legacy Gauge saved under palette A now keeps A’s literal interval colors on a dashboard using palette B. Could dashboard rendering retain the palette-relative indices rather than freezing them before the effective palette is known? ########## superset-frontend/src/explore/components/controls/IntervalColorsControl/legacyColors.ts: ########## @@ -0,0 +1,71 @@ +/** + * 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 { getCategoricalSchemeRegistry } from '@superset-ui/core'; + +export const parseIntervalBounds = (intervals?: string): number[] => + (intervals ?? '') + .split(',') + .map(part => part.trim()) + .filter(part => part !== '') + .map(Number) + .filter(bound => Number.isFinite(bound)); + +/** + * Resolves the legacy `interval_color_indices` control (comma-separated, + * 1-indexed positions into the chosen categorical color scheme) into real + * hex colors, positionally matched to `bounds`. Shared by the control panel + * (display-only fallback while a legacy chart is being edited) and by + * `handleDeprecatedControls` (which migrates the value into `interval_colors` + * on load, since `interval_color_indices` is no longer a registered control + * and would otherwise be silently dropped on the next save). + */ +export const resolveLegacyIntervalColors = ( + bounds: number[], + legacyIntervalColorIndices: string | undefined, + colorScheme: string | undefined, +): string[] => { + // strict: an explicitly-named but unregistered scheme should resolve to + // "no colors" here, not silently fall back to the registry's default + // scheme -- that fallback belongs to the display layer (index.tsx), not + // to this migration helper. + const schemeColors = + getCategoricalSchemeRegistry().get(colorScheme, true)?.colors ?? []; + const indices = (legacyIntervalColorIndices ?? '') + .split(',') + .map(part => part.trim()) + .map(part => (part === '' ? NaN : Number(part))); + // Bounds without an explicit legacy index cycle through the scheme on + // their own counter, independent of their position among the bounds that + // *do* have one -- e.g. indices "2,3" for 3 bounds should leave the third + // (uncovered) bound on the first scheme color, not the third. + let missingCount = 0; + return bounds.map((_, index) => { + const legacyIndex = indices[index]; + if (schemeColors.length === 0) return ''; + if (Number.isFinite(legacyIndex)) { + return schemeColors[ + (((legacyIndex - 1) % schemeColors.length) + schemeColors.length) % + schemeColors.length + ]; + } + const fallbackColor = schemeColors[missingCount % schemeColors.length]; Review Comment: Loading a legacy Gauge with bounds `20,40,60` and indices `2,3` changes its third band: the renderer previously used the third palette color, but this migration uses the first. Could the fallback retain the bound’s original position and the migration test assert identical colors before and after normalization? ########## superset-frontend/plugins/plugin-chart-echarts/src/Bullet/controlPanel.tsx: ########## @@ -59,6 +62,25 @@ const config: ControlPanelConfig = { }, }, ], + [ + { + name: 'range_colors', + config: { + type: 'BulletRangeColorsControl', + label: t('Range colors'), + default: [], + renderTrigger: true, + description: t( + 'Optional custom color for each range band, e.g. to match a brand palette. Bands left unset use the default shading.', + ), Review Comment: Clearing the ranges text while replacing the thresholds hides this control, and `Control`’s default `resetOnHide` behavior immediately resets its custom colors to `[]`; re-entering the ranges cannot restore them, and saving persists the loss. Could the colors survive that temporary hide, with a clear/retype regression test through the Explore control lifecycle? ########## superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts: ########## @@ -74,3 +76,109 @@ export const getHeaderFontSize = (proportionValue: number) => export const getComparisonFontSize = (proportionValue: number) => comparisonFontSizesMapping[proportionValue] ?? sharedFontSizes[sharedFontSizes.length - 1]; + +export interface ComparisonColorTokens { + /** Color for the arrow indicator and (when the symbol is index 0) text. */ + text: string; + /** Background color for the increase/decrease pill. */ + background: string; + /** Foreground color for the increase/decrease pill's text. */ + strongText: string; +} + +/** + * Resolves the increase/decrease colors to use for rendering, given the + * chart's current `increaseColor` / `decreaseColor` (from the + * `ColorPickerControl`s added after this became customizable) and the + * legacy `comparisonColorScheme` field. + * + * Charts saved before `increaseColor` / `decreaseColor` existed only have + * `comparisonColorScheme`, a 2-choice select ('Green' | 'Red') where 'Green' + * meant "green for increase, red for decrease" and 'Red' meant the reverse. + * Both legacy choices map onto the same 'Green' | 'Red' semantic token names + * used by the new controls' presets, so resolving through it here + * reproduces the exact old behavior (including the reversed case) without a + * data migration. + */ +export const resolveComparisonColorKeys = ( + comparisonColorScheme: string | undefined, + increaseColor: string | undefined, + decreaseColor: string | undefined, +): { increaseColor: string; decreaseColor: string } => { + const legacyReversed = comparisonColorScheme === ColorSchemeEnum.Red; + return { + increaseColor: + increaseColor ?? + (legacyReversed ? ColorSchemeEnum.Red : ColorSchemeEnum.Green), + decreaseColor: + decreaseColor ?? + (legacyReversed ? ColorSchemeEnum.Green : ColorSchemeEnum.Red), + }; +}; + +/** + * Hex alpha suffix appended to a custom comparison color to build the pill + * background tint: 0x1A / 0xFF is roughly 10% opacity. + */ +export const COMPARISON_TINT_ALPHA_HEX = '1A'; + +/** + * Resolves a single color value (semantic token name or literal hex from + * the color picker) to the (arrow/text, background, strong-text) triad used + * across the comparison pills. 'Green' / 'Red' keep using the paired + * success/error theme tokens exactly as before these colors were + * customizable; any other value is either a theme token name (e.g. + * 'colorPrimary', emitted by the picker's `resolveThemeTokens` option) or a + * literal hex -- 6-digit, or 8-digit when the alpha-enabled picker is used + * -- in which case the background is a light (~10% opacity) tint of that + * same color. + */ +export const getComparisonColorTokens = ( + colorValue: string, + theme: SupersetTheme, +): ComparisonColorTokens => { + if (colorValue === ColorSchemeEnum.Green) { + return { + text: theme.colorSuccess, + background: theme.colorSuccessBg, + strongText: theme.colorSuccessText, + }; + } + if (colorValue === ColorSchemeEnum.Red) { + return { + text: theme.colorError, + background: theme.colorErrorBg, + strongText: theme.colorErrorText, + }; + } + const themeColors = theme as unknown as Record<string, string>; + const resolvedColor = Object.prototype.hasOwnProperty.call( + themeColors, + colorValue, + ) + ? themeColors[colorValue] + : colorValue; + // An 8-digit hex (alpha-enabled picker) already carries its own alpha + // channel; strip it before appending the tint suffix below so the + // background stays a valid 8-digit hex instead of stacking a second one. + const isEightDigitHex = /^#[0-9a-f]{8}$/i.test(resolvedColor); + const isSixDigitHex = /^#[0-9a-f]{6}$/i.test(resolvedColor); + // Non-hex theme tokens (e.g. an antd token resolving to an `rgba(...)` + // string) can't take a hex alpha suffix without producing invalid CSS, so + // pass those through unchanged rather than tinting them. + if (!isEightDigitHex && !isSixDigitHex) { + return { + text: resolvedColor, + background: resolvedColor, Review Comment: The RGB/RGBA case is fixed, but a theme with `colorPrimary: "#369"` still gives the comparison pill identical foreground and background colors, making its symbols invisible; `Theme.setConfig` preserves that valid shorthand value. Could this normalize supported CSS color formats before generating the tint? ########## superset-frontend/src/explore/components/controls/IntervalColorsControl/index.tsx: ########## @@ -0,0 +1,100 @@ +/** + * 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 '@apache-superset/core/translation'; +import { getCategoricalSchemeRegistry } from '@superset-ui/core'; +import { isRangesInputComplete } from '@superset-ui/plugin-chart-echarts'; +import ControlHeader from '../../ControlHeader'; +import ColorPickerControl from '../ColorPickerControl'; +import type { ColorPickerValue } from '../ColorPickerControl'; +import { + RangeRow as IntervalRow, + RangeLabel as BoundLabel, + replaceColorAtIndex, +} from '../shared/RangeColorRow'; +import { IntervalColorsControlProps } from './types'; +import { + parseIntervalBounds as parseBounds, + resolveLegacyIntervalColors as resolveLegacyColors, +} from './legacyColors'; + +/** + * Per-interval color editor for the Gauge chart. Row *count* is driven by + * the sibling `intervals` control (one row per parsed upper bound) so bound + * values keep a single source of truth; this control only owns colors, + * stored as an array of hex strings positionally matched to those bounds. + */ +export default function IntervalColorsControl({ + value, + onChange, + intervals, + legacyIntervalColorIndices, + colorScheme, + ...headerProps +}: IntervalColorsControlProps) { + const bounds = parseBounds(intervals); + // `parseBounds` leniently drops blank tokens (e.g. mid-edit "20,,60"), + // which would otherwise shift colors to the wrong bound once the blank is + // filled back in -- same hazard `BulletRangeColorsControl` guards against. + const boundsComplete = isRangesInputComplete(intervals); + const legacyColors = resolveLegacyColors( + bounds, + legacyIntervalColorIndices, + colorScheme, + ); + const schemeColors = + getCategoricalSchemeRegistry().get(colorScheme)?.colors ?? []; + + const colorAt = (index: number): string => + value?.[index] || + legacyColors[index] || + schemeColors[index % (schemeColors.length || 1)] || + ''; + + const handleColorChange = (index: number) => (color: ColorPickerValue) => { + if (typeof color !== 'string' || !boundsComplete) return; + onChange?.(replaceColorAtIndex(bounds.length, index, color, colorAt)); + }; + + return ( + <div> + <ControlHeader {...headerProps} /> + {bounds.length === 0 ? ( + <BoundLabel> + {t('Add interval bounds above to configure colors here.')} + </BoundLabel> + ) : ( + bounds.map((bound, index) => ( + // eslint-disable-next-line react/no-array-index-key + <IntervalRow key={index}> Review Comment: Agreed—the split label fixes the number after the translated prefix, so a locale needing the opposite word order cannot express it. Could this use `t("Up to %s", bound)` as the Bullet label does? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
