sadpandajoe commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4303319924
##########
superset-frontend/src/explore/store.ts:
##########
@@ -89,6 +110,55 @@ 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: 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. Same rationale as
+ // the comparison_color_scheme migration above -- `interval_color_indices`
+ // is no longer a registered control, so it's otherwise dropped on save.
+ // Skipped on the dashboard hydration path: the dashboard's `color_scheme`
+ // override is applied after this runs, so freezing literal colors here would
+ // pin the chart's own palette. The renderer resolves the legacy indices
+ // against the effective palette instead.
+ if (
+ resolveLegacyGaugeColors &&
+ formData.viz_type === VizType.Gauge &&
+ formData.interval_color_indices &&
+ (!formData.interval_colors || formData.interval_colors.length === 0)
+ ) {
+ const bounds = parseIntervalBounds(formData.intervals);
+ const resolved = resolveLegacyIntervalColors(
Review Comment:
Opening a legacy Gauge in Explore now turns the palette-relative
`interval_color_indices` into literal `interval_colors` using the chart's own
`color_scheme`, even if the user never touches interval colors. The renderer
prefers `interval_colors` whenever it is populated, so after an unrelated save
a dashboard that overrides the palette with a different scheme stops following
that override. Skipping the migration on dashboard hydration doesn't cover this
resave path. Should the legacy indices be kept as-is until the user explicitly
edits an interval color?
##########
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',
config: {
- type: 'SelectControl',
- label: t('color scheme for comparison'),
- default: ColorSchemeEnum.Green,
- renderTrigger: true,
- choices: [
- [ColorSchemeEnum.Green, 'Green for increase, red for
decrease'],
- [ColorSchemeEnum.Red, 'Red for increase, green for decrease'],
- ],
- visibility: ({ controls }) =>
- controls?.comparison_color_enabled?.value === true,
+ ...comparisonColorControlConfig,
+ label: t('Color for increase'),
Review Comment:
The old `SelectControl` gave screen-reader users a name from its label, but
`ColorPickerControl` only sets `aria-label` from the `ariaLabel` prop and these
two pickers don't pass one (the Gauge and Bullet controls do). Assistive tech
will announce the color value rather than 'Color for increase' / 'Color for
decrease'. Could `ariaLabel` be added to the shared config or per control?
##########
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 ?? [];
+ const indices = (legacyIntervalColorIndices ?? '')
+ .split(',')
Review Comment:
This resolver handles each index token on its own, whereas
`getIntervalBoundsAndColors` in `Gauge/transformProps.ts` discards the whole
indices list when any token fails to parse. With bounds `20,60`, indices
`2,invalid` and a scheme `[A,B,C]`, the renderer shows `[A,B]` today, but
opening the chart in Explore migrates and a save persists `[B,B]` with no color
interaction. Could the migration and the renderer share one legacy resolver so
a save can't reinterpret existing state?
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/colors.ts:
##########
@@ -0,0 +1,63 @@
+/**
+ * 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 type { RgbaColor } from '@superset-ui/core';
+
+const HEX_COLOR = /^#([0-9a-f]{3}|[0-9a-f]{6})$/i;
Review Comment:
`ColorPickerControl` emits 8-digit hex (`#rrggbbaa`) whenever the user picks
a non-opaque color, but this pattern only accepts 3 or 6 digits. If someone
picks `#33669980` for the Big Number comparison color and then switches the viz
to Waterfall or Candlestick, `normalizeRgbaColor` falls back to the default
color while the control still shows the custom one. Should the 8-digit form be
accepted here and its alpha parsed?
--
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]
