sadpandajoe commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4151337845
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts:
##########
@@ -100,21 +125,24 @@ const config: ControlPanelConfig = {
],
[
{
- name: 'comparison_color_scheme',
+ name: 'increase_color',
Review Comment:
Opening a saved chart with `comparison_color_scheme: 'Red'` in Explore now
reverses its increase/decrease colors: `getFormDataFromControls` only
serializes registered controls, so removing the old control drops the scheme
before `resolveComparisonColorKeys` can use it, and saving persists that reset.
Could we retain the legacy field or migrate its value into the new controls
during initialization?
##########
superset-frontend/src/explore/components/controls/IntervalColorsControl/index.tsx:
##########
@@ -0,0 +1,132 @@
+/**
+ * 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 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';
+
+const parseBounds = (intervals?: string): number[] =>
+ (intervals ?? '')
+ .split(',')
+ .map(part => part.trim())
+ .filter(part => part !== '')
Review Comment:
Temporarily changing bounds from `20,40,60` to `20,,60` shifts the editor's
rows even though the renderer rejects that list; changing the displayed 60 row
then writes a two-color array, drops the third color, and attaches the edit to
40 when that bound is restored. Could the editor share the renderer's
parsing/validity rule and avoid persisting color edits while the bounds are
invalid?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx:
##########
@@ -278,15 +279,26 @@ const config: ControlPanelConfig = {
],
[
{
- name: 'interval_color_indices',
+ name: 'interval_colors',
config: {
- type: 'TextControl',
+ type: 'IntervalColorsControl',
label: t('Interval colors'),
description: t(
- 'Comma-separated color picks for the intervals, e.g. 1,2,4.
Integers denote colors from the chosen color scheme and are 1-indexed. Length
must be matching that of interval bounds.',
+ 'Pick a color for each interval band defined above by its
upper bound. Charts saved with the legacy 1-indexed "Interval colors" text
field are automatically resolved against the chosen color scheme the first time
this panel is opened.',
),
renderTrigger: true,
- default: DEFAULT_FORM_DATA.intervalColorIndices,
+ default: DEFAULT_FORM_DATA.intervalColors,
+ shouldMapStateToProps: () => true,
+ mapStateToProps: (state: ControlPanelState) => ({
+ intervals: state?.controls?.intervals?.value as string,
+ // `interval_color_indices` is no longer a registered control
+ // (replaced by this one), so it never appears under
+ // `state.controls`. Read it from the raw persisted
Review Comment:
This preserves the legacy colors in the picker display, but not in Explore's
render/save data: a chart saved with `interval_color_indices: '2,3'` still has
`interval_colors: []`, and `getFormDataFromControls` drops the unregistered
legacy field, so its preview uses scheme colors 1 and 2 and saving can discard
the original picks. Could we preserve the old control or initialize the new
control's value from those indices before serialization?
##########
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:
For a theme token resolving to `rgb(...)` or `rgba(...)`, this assigns the
same color to the comparison symbol's foreground and background, making the
symbol invisible or very low-contrast instead of giving it a light tint. Could
the background tint be generated independently of the input color format?
--
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]
