sadpandajoe commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4298535930
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts:
##########
@@ -102,23 +111,42 @@ export default function transformProps(
theme.colorFillSecondary,
theme.colorFill,
];
+ // Custom colors (if any) are authored positionally against the original,
+ // pre-sort `ranges` order via the `range_colors` control -- capture each
+ // range's color here, before the descending sort below reorders them.
const sortedRanges = [...ranges]
- .map((value, i) => ({ value, label: rangeLabels[i] }))
+ .map((value, i) => ({
+ value,
+ label: rangeLabels[i],
+ color: rangeColors?.[i] || undefined,
+ }))
.filter(({ value }) => value > axisMin)
.sort((a, b) => b.value - a.value);
// Each band's label sits inside its own top-right corner: the visible strip
// of a nested band ends at its threshold, so the label lands inside the
// range it names and the rightmost label cannot clip at the chart edge.
- const bands = sortedRanges.map(({ value, label }, i) => ({
+ const getBandLabelColor = (fill?: string): string => {
+ const luminance = fill ? getColorLuminance(fill) : null;
+ if (luminance === null) return theme.colorTextSecondary;
+ // Fixed light/dark foregrounds, not `theme.colorText`: that token flips
+ // to a light color in dark themes and would vanish on a light custom fill.
+ return luminance < 0.4 ? LIGHT_LABEL_COLOR : DARK_LABEL_COLOR;
Review Comment:
`getColorLuminance` returns WCAG relative luminance, where white and the
`#1f1f1f` foreground break even around 0.21, but the cutoff here is 0.4. With
Show labels on and a custom band fill of `#999999` (luminance ≈ 0.32) the label
is white at about 2.85:1 contrast, versus about 5.8:1 with the dark foreground,
so common mid-tone fills get hard-to-read labels. Should the threshold be
derived from the contrast ratio against each candidate foreground instead?
##########
superset-frontend/src/explore/components/controls/IntervalColorsControl/legacyColors.ts:
##########
@@ -0,0 +1,64 @@
+/**
+ * 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[] => {
+ // Non-strict lookup mirrors the Gauge renderer: an unregistered scheme
+ // falls back to the registry's default scheme.
+ const schemeColors =
+ getCategoricalSchemeRegistry().get(colorScheme)?.colors ?? [];
Review Comment:
This resolves legacy indices against the registry's original palette, but
the Gauge renderer indexes `colorFn.colors` after grouped-series labels have
already expanded it when `USE_ANALOGOUS_COLORS` is on. For a saved Gauge with
`color_scheme: 'echarts5Colors'` (9 colors), ten grouped rows, `intervals:
'50,100'` and `interval_color_indices: '10,2'`, the old render used the first
generated analogous color for index 10; the migration now wraps modulo 9 to
`#5470C6`, and saving persists that wrong band color. Could the indices stay
palette-relative until the effective palette is known, or be resolved against
the same expanded palette the renderer uses?
##########
superset-frontend/src/features/themes/ThemeColorPickers.tsx:
##########
@@ -0,0 +1,175 @@
+/**
+ * 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 { useMemo } from 'react';
+import { t } from '@apache-superset/core/translation';
+import { styled } from '@apache-superset/core/theme';
+import { Typography } from '@superset-ui/core/components/Typography';
+import ColorPickerControl from
'src/explore/components/controls/ColorPickerControl';
+import type { ColorPickerValue } from
'src/explore/components/controls/ColorPickerControl';
+
+/**
+ * Curated antd theme SEED tokens (plus a handful of the most load-bearing
+ * MapToken/AliasToken derivatives) surfaced as individual color pickers, so
+ * the common case -- "change the brand color" -- doesn't require pasting a
+ * hand-edited JSON blob. This intentionally does not attempt to cover the
+ * full antd token surface (100+ tokens): anything not listed here is still
+ * fully editable via the JSON textarea below, which remains the source of
+ * truth. Names are taken directly from antd's own `SeedToken` /
+ * `MapToken` types -- see `antd/es/theme/interface/{seeds,maps/colors}.d.ts`
+ * -- never invented.
+ */
+export const CURATED_COLOR_TOKENS = [
+ { key: 'colorPrimary', label: () => t('Primary') },
+ { key: 'colorSuccess', label: () => t('Success') },
+ { key: 'colorWarning', label: () => t('Warning') },
+ { key: 'colorError', label: () => t('Error') },
+ { key: 'colorInfo', label: () => t('Info') },
+ { key: 'colorLink', label: () => t('Link') },
+ { key: 'colorText', label: () => t('Text') },
+ { key: 'colorTextSecondary', label: () => t('Secondary text') },
+ { key: 'colorBgBase', label: () => t('Base background') },
+ { key: 'colorBgContainer', label: () => t('Container background') },
+ { key: 'colorBorder', label: () => t('Border') },
+] as const;
+
+export type CuratedColorToken = (typeof CURATED_COLOR_TOKENS)[number]['key'];
+
+interface ParsedThemeJson {
+ token?: Record<string, unknown>;
+ [key: string]: unknown;
+}
+
+const isPlainObject = (value: unknown): value is Record<string, unknown> =>
+ typeof value === 'object' && value !== null && !Array.isArray(value);
+
+/** Attempts to parse `jsonData` as a theme config object; returns `null`
+ * (never throws) when the JSON is empty, mid-edit, or otherwise invalid --
+ * matching the JSON-parse error handling already used elsewhere in
+ * ThemeModal (`formatJsonData`, `isValidJson`). A `token` that parses but
+ * isn't itself a plain object (a string, array, or number) also counts as
+ * invalid here, since `patchThemeJsonToken` below spreads it -- and
+ * spreading a string or array silently rewrites it into numeric-keyed
+ * junk rather than the token map callers expect. */
+export const tryParseThemeJson = (
+ jsonData: string | undefined,
+): ParsedThemeJson | null => {
+ if (!jsonData?.trim()) return null;
+ try {
+ const parsed = JSON.parse(jsonData);
+ if (!isPlainObject(parsed)) return null;
+ if (
+ 'token' in parsed &&
+ parsed.token !== undefined &&
+ !isPlainObject(parsed.token)
+ ) {
+ return null;
+ }
+ return parsed;
+ } catch {
+ return null;
+ }
+};
+
+/** Patches a single curated token into `jsonData`'s `token` object,
+ * preserving every other key (including tokens this UI doesn't curate) and
+ * re-serializing with the same 2-space indent used throughout this modal. */
+export const patchThemeJsonToken = (
+ jsonData: string,
+ key: CuratedColorToken,
+ value: string,
+): string => {
+ const parsed = tryParseThemeJson(jsonData) ?? {};
+ const next = {
+ ...parsed,
+ token: {
+ ...parsed.token,
+ [key]: value,
+ },
+ };
+ return JSON.stringify(next, null, 2);
+};
+
+const TokenGrid = styled.div`
+ display: grid;
+ grid-template-columns: repeat(auto-fill, minmax(220px, 1fr));
+ gap: ${({ theme }) => theme.sizeUnit * 3}px;
+ margin-bottom: ${({ theme }) => theme.sizeUnit * 4}px;
+`;
+
+const TokenRow = styled.div`
+ display: flex;
+ align-items: center;
+ justify-content: space-between;
+ gap: ${({ theme }) => theme.sizeUnit * 2}px;
+`;
+
+interface ThemeColorPickersProps {
+ jsonData: string;
+ onChange: (nextJsonData: string) => void;
+ disabled?: boolean;
+}
+
+export default function ThemeColorPickers({
+ jsonData,
+ onChange,
+ disabled = false,
+}: ThemeColorPickersProps) {
+ const parsed = useMemo(() => tryParseThemeJson(jsonData), [jsonData]);
+ const isValid = parsed !== null;
+ const tokenValues = parsed?.token ?? {};
+
+ const handleTokenChange =
+ (key: CuratedColorToken) => (color: ColorPickerValue) => {
+ if (typeof color !== 'string' || !isValid || disabled) return;
+ onChange(patchThemeJsonToken(jsonData, key, color));
+ };
+
+ return (
+ <div data-test="theme-color-pickers">
+ {!isValid && (
+ <Typography.Text
+ type="secondary"
+ data-test="theme-color-pickers-invalid-json-notice"
+ >
+ {t(
+ 'Fix the JSON errors below to edit colors here; your changes ' +
+ 'sync automatically once the JSON is valid again.',
+ )}
+ </Typography.Text>
+ )}
+ <TokenGrid>
+ {CURATED_COLOR_TOKENS.map(({ key, label }) => {
+ const tokenValue = tokenValues[key];
+ return (
+ <TokenRow key={key} data-test={`theme-color-picker-${key}`}>
+ <Typography.Text>{label()}</Typography.Text>
+ <ColorPickerControl
+ ariaLabel={label()}
+ value={typeof tokenValue === 'string' ? tokenValue : undefined}
Review Comment:
Typing a raw string that happens to match a numeric theme key into the JSON,
e.g. `{"token":{"colorPrimary":"fontSize"}}`, passes the `typeof === 'string'`
guard here, then `ColorPickerControl.toDisplayHex` resolves it as a theme-token
name and calls `.toLowerCase()` on the numeric `theme.fontSize`. That throws
during render, before Apply or Save, and takes down the modal. Should the
picker value be validated as a color (or token resolution guarded for
non-string values) so mid-edit JSON stays editable?
##########
superset-frontend/src/explore/components/controls/ColorPickerControl.test.tsx:
##########
@@ -238,3 +238,10 @@ test('handles undefined theme gracefully', () => {
expect(() => render(<ColorPickerControl {...defaultProps} />)).not.toThrow();
});
+
+test('trigger is exposed as a focusable button for keyboard users', () => {
+ const { container } = render(<ColorPickerControl {...defaultProps} />);
+ const trigger = container.querySelector('.ant-color-picker-trigger');
+ expect(trigger).toHaveAttribute('tabindex', '0');
Review Comment:
This test only asserts `tabindex` and `role`, so deleting the new
Enter/Space `keydown` handler would leave it green while keyboard users could
no longer open the picker. Could it focus the trigger, press Enter and Space
and assert the popup opens, and also assert a `disabled` trigger stays closed?
##########
superset-frontend/src/explore/store.test.tsx:
##########
@@ -373,3 +385,108 @@ test('handleDeprecatedControls is idempotent — no-op when
matrixify_enable alr
expect(formData.matrixify_mode_rows).toBe('dimensions');
expect(formData.matrixify_mode_columns).toBe('metrics');
});
+
+// ============================================================
+// #42910: legacy color-picker field migrations. `comparison_color_scheme`
+// and `interval_color_indices` are no longer registered controls, so
+// `getFormDataFromControls` drops them on the next save — these migrations
+// carry their value forward into the controls that replaced them before
+// that can happen.
+// ============================================================
+
+test('handleDeprecatedControls migrates comparison_color_scheme Green into
increase/decrease colors', () => {
+ const formData: any = {
+ viz_type: VizType.BigNumberPeriodOverPeriod,
+ comparison_color_scheme: 'Green',
+ };
+ handleDeprecatedControls(formData);
+
+ expect(formData.increase_color).toBe('Green');
+ expect(formData.decrease_color).toBe('Red');
+});
+
+test('handleDeprecatedControls migrates the reversed comparison_color_scheme
Red choice', () => {
+ const formData: any = {
+ viz_type: VizType.BigNumberPeriodOverPeriod,
+ comparison_color_scheme: 'Red',
+ };
+ handleDeprecatedControls(formData);
+
+ expect(formData.increase_color).toBe('Red');
+ expect(formData.decrease_color).toBe('Green');
+});
+
+test('handleDeprecatedControls does not override already-set increase/decrease
colors', () => {
+ const formData: any = {
+ viz_type: VizType.BigNumberPeriodOverPeriod,
+ comparison_color_scheme: 'Red',
+ increase_color: '#336699',
+ };
+ handleDeprecatedControls(formData);
+
+ expect(formData.increase_color).toBe('#336699');
+ expect(formData.decrease_color).toBeUndefined();
+});
+
+test('handleDeprecatedControls is a no-op for BigNumberPeriodOverPeriod
without a legacy scheme', () => {
+ const formData: any = { viz_type: VizType.BigNumberPeriodOverPeriod };
+ handleDeprecatedControls(formData);
+
+ expect(formData.increase_color).toBeUndefined();
+ expect(formData.decrease_color).toBeUndefined();
+});
+
+test('handleDeprecatedControls migrates interval_color_indices into
interval_colors', () => {
+ const formData: any = {
+ viz_type: VizType.Gauge,
+ intervals: '20,40,60',
+ interval_color_indices: '2,3',
+ color_scheme: 'testScheme',
+ };
+ handleDeprecatedControls(formData);
+
+ // The uncovered third bound keeps its positional scheme color, matching
+ // what the Gauge renderer drew before the migration.
+ expect(formData.interval_colors).toEqual(['#ff7f0e', '#2ca02c', '#2ca02c']);
+});
+
+test('applyDefaultFormData keeps legacy interval_color_indices unresolved for
dashboards', () => {
+ // The dashboard's color_scheme override is applied after hydration, so the
+ // legacy palette-relative indices must not be frozen into literal colors.
+ const result = applyDefaultFormData({
+ viz_type: VizType.Gauge,
+ datasource: '1__table',
+ intervals: '20,40,60',
+ interval_color_indices: '2,3',
+ color_scheme: 'testScheme',
+ } as any);
+
+ expect(result.interval_color_indices).toBe('2,3');
+ expect(result.interval_colors).toEqual([]);
Review Comment:
This assertion fails as written (CI shows `Expected: []`, `Received:
undefined` at this line). The test only registers `test-chart`, so
`getAllControlsState` never sees Gauge's control panel and cannot supply the
`interval_colors: []` default. Can the real Gauge control panel be registered
(and removed afterward) so the hydration case exercises the actual default?
##########
superset-frontend/plugins/plugin-chart-echarts/test/utils/colors.test.ts:
##########
@@ -0,0 +1,48 @@
+/**
+ * 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 { normalizeRgbaColor } from '../../src/utils/colors';
+
+const fallback = { r: 1, g: 2, b: 3, a: 1 };
+
+test('passes through an rgb object', () => {
+ expect(
+ normalizeRgbaColor({ r: 10, g: 20, b: 30, a: 0.5 }, fallback),
+ ).toEqual({ r: 10, g: 20, b: 30, a: 0.5 });
+});
+
+test('parses 6 and 3 digit hex strings', () => {
Review Comment:
These tests cover `normalizeRgbaColor` in isolation, but not the
Waterfall/Candlestick fix itself: the existing transform tests pass RGB
objects, so reverting either chart's `normalizeRgbaColor` call would still
pass. Could a test feed a carried-over hex value such as `increase_color:
'#336699'` through each chart's `transformProps` and assert the generated
series color?
--
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]
