rusackas commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4333244523
##########
superset-frontend/src/explore/store.ts:
##########
@@ -89,6 +110,63 @@ 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: on request, 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. Defaults to off
+ // (unlike the comparison_color_scheme migration above): unlike that
+ // control, `interval_color_indices` is never dropped from `form_data` by
+ // an unrelated save -- it isn't a registered control, but `form_data` is
+ // persisted as a plain object, not rebuilt from registered controls, so
+ // an untouched legacy string survives save as-is. Eagerly resolving it
+ // merely from opening the chart (the old default here) froze it into
+ // literal colors on the *first* unrelated resave too, silently breaking a
+ // dashboard override of this chart's `color_scheme` applied afterward --
+ // the exact hazard the dashboard hydration path below was already written
+ // to avoid, just not for this path. `IntervalColorsControl` already shows
+ // the resolved colors for display on its own (see its own
+ // `legacyIntervalColorIndices` prop), so the indices stay palette-relative
+ // and keep following the effective palette until the user actually edits
+ // an interval color, which is the only point this needs to be literal.
+ if (
+ resolveLegacyGaugeColors &&
Review Comment:
Good catch, Explore does rebuild the query/save form data from registered
controls, so the indices never reached the renderer there. Went with your
hidden-control idea: `interval_color_indices` stays registered on the Gauge
panel, so an untouched legacy chart keeps rendering and saving with its
palette-relative colors until the first color edit populates `interval_colors`.
##########
superset/mcp_service/chart/preview_utils.py:
##########
@@ -2799,9 +2834,18 @@ def threshold_details(
"x": {"datum": axis_min, "type": "quantitative"},
"x2": {"datum": threshold},
"y": y_encoding,
- "color": legend_color(
- (f"{label}: " if label else "")
- + f"≤ {_format_bullet_number(model.y_axis_format,
threshold)}"
+ # A custom `range_colors` entry wins; otherwise fall back
+ # to the default categorical legend coloring exactly as
+ # before this control existed, so a Bullet chart saved
+ # without `range_colors` previews unchanged.
+ "color": (
+ {"value": color}
Review Comment:
Good call. Switched it to ride on the same `datum` encoding through a
per-layer scale (`resolve.scale.color` is already independent), so
custom-colored ranges keep their legend entries. Added a test for the
`show_legend` plus `range_labels` case you described.
##########
superset-frontend/src/explore/components/controls/IntervalColorsControl/IntervalColorsControl.test.tsx:
##########
@@ -0,0 +1,284 @@
+/**
+ * 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 {
+ render,
+ screen,
+ userEvent,
+ waitFor,
+} from 'spec/helpers/testing-library';
+import {
+ CategoricalScheme,
+ getCategoricalSchemeRegistry,
+} from '@superset-ui/core';
+import IntervalColorsControl from
'src/explore/components/controls/IntervalColorsControl';
+
+beforeAll(() => {
+ getCategoricalSchemeRegistry().registerValue(
+ 'testScheme',
+ new CategoricalScheme({
+ id: 'testScheme',
+ colors: ['#1f77b4', '#ff7f0e', '#2ca02c'],
+ }),
+ );
+});
+
+beforeEach(() => {
+ jest.clearAllMocks();
+});
+
+test('renders a hint and no rows when no interval bounds are configured', ()
=> {
+ render(<IntervalColorsControl onChange={jest.fn()} />);
+
+ expect(
+ screen.getByText('Add interval bounds above to configure colors here.'),
+ ).toBeInTheDocument();
+ expect(document.querySelectorAll('.ant-color-picker-trigger')).toHaveLength(
+ 0,
+ );
+});
+
+test('renders one color picker row per parsed interval bound', () => {
+ render(
+ <IntervalColorsControl
+ onChange={jest.fn()}
+ intervals="20,60,100"
+ colorScheme="testScheme"
+ />,
+ );
+
+ expect(screen.getByText(/Up to 20/)).toBeInTheDocument();
+ expect(screen.getByText(/Up to 60/)).toBeInTheDocument();
+ expect(screen.getByText(/Up to 100/)).toBeInTheDocument();
+ expect(document.querySelectorAll('.ant-color-picker-trigger')).toHaveLength(
+ 3,
+ );
+});
+
+test('falls back to the categorical scheme when there is no explicit value or
legacy data', () => {
+ render(
+ <IntervalColorsControl
+ onChange={jest.fn()}
+ intervals="20,60"
+ colorScheme="testScheme"
+ />,
+ );
+
+ expect(screen.getByText('#1F77B4')).toBeInTheDocument();
+ expect(screen.getByText('#FF7F0E')).toBeInTheDocument();
+});
+
+test('resolves legacy interval_color_indices against the color scheme for
display', () => {
+ render(
+ <IntervalColorsControl
+ onChange={jest.fn()}
+ intervals="20,60"
+ legacyIntervalColorIndices="2,3"
+ colorScheme="testScheme"
+ />,
+ );
+
+ // 1-indexed: index 2 -> '#ff7f0e', index 3 -> '#2ca02c'
+ expect(screen.getByText('#FF7F0E')).toBeInTheDocument();
+ expect(screen.getByText('#2CA02C')).toBeInTheDocument();
+});
+
+test('disables editing while intervals has a blank token between numbers', ()
=> {
+ render(
+ <IntervalColorsControl
+ onChange={jest.fn()}
+ intervals="20,,60"
+ colorScheme="testScheme"
+ />,
+ );
+
+ // The blank middle token means the list is still being edited; don't
+ // allow commits that would misalign colors once the blank is filled in.
+ document.querySelectorAll('.ant-color-picker-trigger').forEach(trigger => {
Review Comment:
Yep, that could pass without a single `expect` running. It now pins two
triggers first, then clicks one and checks the picker doesn't open and
`onChange` isn't called.
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.test.tsx:
##########
@@ -0,0 +1,107 @@
+/**
+ * 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 { render, screen } from 'spec/helpers/testing-library';
+import PopKPI from './PopKPI';
+import { PopKPIProps } from './types';
+
+const baseProps: PopKPIProps = {
+ height: 200,
+ width: 400,
+ data: [],
+ metrics: [],
+ metricName: 'Sales',
+ showMetricName: true,
+ headerText: '',
+ boldText: true,
+ bigNumber: '120',
+ prevNumber: '100',
+ valueDifference: '20',
+ percentDifferenceFormattedString: '20%',
+ compType: 'r',
+ percentDifferenceNumber: 0.2,
+ subtitleFontSize: 0.15,
+ shift: '',
+ headerFontSize: 'l',
+ subheaderFontSize: 'm',
+ comparisonColorEnabled: true,
+};
+
+test('renders the up arrow and metric values when the metric increased', () =>
{
+ render(<PopKPI {...baseProps} />);
+
+ expect(screen.getByText('↑')).toBeInTheDocument();
+ expect(screen.getByText('120')).toBeInTheDocument();
+ expect(screen.getByText('Sales')).toBeInTheDocument();
+});
+
+test('renders the down arrow when the metric decreased', () => {
+ render(
+ <PopKPI
+ {...baseProps}
+ percentDifferenceNumber={-0.2}
+ valueDifference="-20"
+ />,
+ );
+
+ expect(screen.getByText('↓')).toBeInTheDocument();
+ expect(screen.getByText('-20')).toBeInTheDocument();
+});
+
+// The precise color resolution (legacy `comparisonColorScheme` fallback,
+// custom hex precedence, the neutral disabled state) is covered by pure
+// unit tests against `resolveComparisonColorKeys` / `getComparisonColorTokens`
+// in utils.test.ts, since emotion's `css` prop styles aren't reliably
+// observable through jsdom's computed styles. These tests instead confirm
+// PopKPI renders without crashing across the same prop combinations.
+test('renders without crashing with only the legacy comparisonColorScheme
(backward compatibility)', () => {
+ render(<PopKPI {...baseProps} comparisonColorScheme="Red" />);
+
+ expect(screen.getByText('↑')).toBeInTheDocument();
+});
+
+test('renders without crashing with a custom increaseColor hex', () => {
Review Comment:
Agreed, those only proved the arrow exists. The actual snag was that jest's
babel config compiles the `css` prop with React's own JSX runtime (webpack goes
through swc with `@emotion/react`), so the span's style was silently dropped in
tests, hence the earlier 'Property not found: color'. Made the arrow a styled
span like the pill, and the tests now assert the arrow and pill colors for the
legacy scheme, a custom hex, and the disabled case.
##########
superset-frontend/src/explore/components/controls/IntervalColorsControl/index.tsx:
##########
@@ -0,0 +1,114 @@
+/**
+ * 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,
+ hasTrailingBlankToken,
+} 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);
+ // See the identical guard in `BulletRangeColorsControl`: while `intervals`
+ // ends in the one tolerated trailing blank (the last bound is being
+ // retyped), `bounds` is transiently one entry short. Writing a color at
+ // that moment must not truncate the array to that shorter length, or the
+ // color for the bound being retyped is permanently lost.
+ const writeLength = hasTrailingBlankToken(intervals)
Review Comment:
Good catch. Went with resolving for the full previous length: while
`intervals` ends in the trailing blank and there are no explicit colors yet,
the legacy index count now sets the floor for the written array, so the band
being retyped keeps its legacy color. Added a test for exactly the `20,40,60` /
`3,2,1` sequence.
--
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]
