sadpandajoe commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4180049016
##########
superset-frontend/plugins/plugin-chart-echarts/src/Gauge/types.ts:
##########
@@ -50,7 +50,21 @@ export type EchartsGaugeFormData = QueryFormData & {
endAngle: number;
showPointer: boolean;
intervals: string;
+ /**
+ * @deprecated Legacy 1-indexed positions into `colorScheme`, e.g. "1,2,4".
+ * Superset versions before `intervalColors` existed stored interval colors
+ * this way. Still read at render time (see
+ * `transformProps.ts#getIntervalBoundsAndColors`) so charts saved with
+ * this shape keep rendering identically without a migration.
+ */
intervalColorIndices: string;
+ /**
+ * Real hex/rgb colors for each interval band, positionally matched to the
+ * bounds parsed from `intervals`. Authored via the `IntervalColorsControl`
+ * control panel row. Takes precedence over `intervalColorIndices` when
+ * present.
+ */
+ intervalColors?: string[];
Review Comment:
Rebinding a Gauge's dataset through MCP `update_chart` drops these custom
colors: `_GAUGE_PRESENTATION_FORM_DATA_KEYS` preserves `interval_color_indices`
but not `interval_colors`, and the replacement params are saved without them.
Could the new field be preserved during rebinding, with a regression asserting
the picked colors survive?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts:
##########
@@ -53,29 +53,52 @@ import { getColtypesMapping } from '../utils/series';
export const getIntervalBoundsAndColors = (
intervals: string,
intervalColorIndices: string,
+ intervalColors: string[] | undefined,
colorFn: CategoricalColorScale,
min: number,
max: number,
): Array<[number, string]> => {
let intervalBoundsNonNormalized;
- let intervalColorIndicesArray;
try {
intervalBoundsNonNormalized = parseNumbersList(intervals, ',');
- intervalColorIndicesArray = parseNumbersList(intervalColorIndices, ',');
} catch (error) {
intervalBoundsNonNormalized = [] as number[];
- intervalColorIndicesArray = [] as number[];
}
const intervalBounds = intervalBoundsNonNormalized.map(
bound => (bound - min) / (max - min),
);
- const intervalColors = intervalColorIndicesArray.map(
+
+ // New-style: `interval_colors` holds real hex/rgb colors chosen through
+ // the Gauge control panel's per-interval `IntervalColorsControl`,
+ // positionally matched to the bounds parsed from `intervals`. Prefer this
+ // whenever it's populated.
+ if (Array.isArray(intervalColors) && intervalColors.length > 0) {
+ return intervalBounds.map((val, idx) => [
+ val,
Review Comment:
A saved Gauge now uses `interval_colors` here, but its MCP Vega-Lite preview
still reads only `interval_color_indices` in `_prepare_gauge_preview`, so
custom red/green bands appear in the default palette instead. Could the preview
honor the same explicit-color precedence?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts:
##########
@@ -102,17 +103,27 @@ 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 bands = sortedRanges.map(({ value, label, color }, i) => ({
name: label,
itemStyle: {
- color: bandFills[Math.min(i, bandFills.length - 1)],
+ // A custom `range_colors` entry wins; otherwise fall back to the
+ // default theme-token ramp exactly as before this control existed, so
+ // Bullet charts saved without `range_colors` are unaffected.
+ color: color || bandFills[Math.min(i, bandFills.length - 1)],
Review Comment:
Choosing a black range fill with Show labels enabled makes its label
unreadable in the light theme: the label sits inside the band and still uses
the dark `colorTextSecondary` foreground. Could custom fills get a contrasting
label color or place the label outside the fill?
##########
superset-frontend/src/explore/components/controls/IntervalColorsControl/index.tsx:
##########
@@ -0,0 +1,98 @@
+/**
+ * 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] ||
Review Comment:
With `interval_colors: ['#ff0000']` and legacy indices `3,1`, the renderer
uses the second palette color for the missing second entry, but this picker
displays the first; editing only the first row then persists that different
color into the untouched second band. Could populated new arrays use the
renderer's positional fallback instead of resurrecting legacy indices?
--
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]
