sadpandajoe commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4320746382
##########
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:
With the Gauge migration off by default, opening a legacy Gauge in Explore
builds its render and save data from the registered controls, and
`interval_color_indices` is no longer one (the Gauge control panel dropped it).
A chart saved with a `[A,B,C]` palette, `intervals: "20,40"` and
`interval_color_indices: "2,3"` therefore previews with positional colors
`[A,B]` instead of `[B,C]`, and a resave drops the indices, so the stored chart
changes colors without the user touching the Interval colors control. Should
the indices stay in the form data (for example a hidden control) until an
interval color is actually edited?
##########
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:
A custom `range_colors` entry swaps this layer's color encoding for a fixed
`{"value": color}`, which bypasses `legend_color`. With `ranges: "50,100"`,
`range_labels: "Low,High"`, `show_legend: true` and `range_colors:
["#ff0000","#00ff00"]`, the preview legend lists only the metric and both range
entries disappear, while a chart without custom colors keeps them. Could the
custom colors be applied through a scale on the legend-bearing encoding so the
range labels stay in the legend?
##########
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:
Every assertion in this test sits inside
`querySelectorAll(...).forEach(...)`, so if the control rendered no pickers for
`intervals="20,,60"` the test would still pass without running one `expect`.
Could it first assert that two `.ant-color-picker-trigger` elements exist, and
then check that a change attempt does not call `onChange`?
##########
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:
The out-of-palette case still changes untouched bands once any row is
edited. With a 3-color scheme, `intervals: "50,100,150"` and
`interval_color_indices: "10,2"`, `resolveLegacyIntervalColors` now returns
blanks, but `colorAt` in `IntervalColorsControl/index.tsx` falls through to
`schemeColors[index % n]`, and `replaceColorAtIndex` writes those static colors
for every other row. Editing only the second row therefore persists the static
first palette color for the first band, replacing the color the renderer
produced for index 10, and `interval_colors` then takes precedence over the
legacy indices. Could `colorAt` leave unresolved legacy bands untouched, for
example by not writing explicit colors until all rows are resolved?
--
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]
