rusackas commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4309437209
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts:
##########
@@ -35,6 +36,30 @@ import {
metricNameFontSizeWithVisibility,
} from '../sharedControls';
+// Shared base for the `increase_color` / `decrease_color` pickers so the
+// presets, token resolution, output format and visibility rule can't drift
+// apart. No static default: charts saved before these controls existed only
+// have `comparison_color_scheme` ('Green' | 'Red', where 'Red' reverses
+// increase/decrease colors). Leaving each control's value undefined lets
+// `resolveComparisonColorKeys` (see BigNumberPeriodOverPeriod/utils.ts)
+// resolve the correct color from that legacy scheme at render time. A
+// hardcoded default here would win over the legacy fallback via
+// `applyDefaultFormData` and silently repaint old dashboards.
+const comparisonColorControlConfig: CustomControlItem['config'] = {
+ type: 'ColorPickerControl',
+ renderTrigger: true,
+ presets: [
+ {
+ label: t('Semantic colors'),
+ colors: [ColorSchemeEnum.Green, ColorSchemeEnum.Red],
Review Comment:
Right, the picker was always showing SPECIAL_COLORS'\'' fixed muted green
for '\''Green'\'' regardless of what colorSuccess actually resolves to. It now
checks for a matching theme token first and shows that, falling back to the
fixed preset color only when there'\''s no live token to resolve against.
Should keep the swatch honest even if colorSuccess gets restyled.
##########
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:
You'\''re right, and I think the comment'\''s own rationale here doesn'\''t
actually hold up. interval_color_indices is not dropped by a save even without
this migration, form_data gets persisted as a plain object rather than rebuilt
from registered controls, so an untouched legacy string survives a save on its
own. So I just turned the eager resolution off by default, same as the
dashboard path already does, and only materialize literal colors once the user
actually touches a color in the picker.
--
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]
