sadpandajoe commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4250508652
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts:
##########
@@ -74,3 +76,126 @@ export const getHeaderFontSize = (proportionValue: number)
=>
export const getComparisonFontSize = (proportionValue: number) =>
comparisonFontSizesMapping[proportionValue] ??
sharedFontSizes[sharedFontSizes.length - 1];
+
+export interface ComparisonColorTokens {
+ /** Color for the arrow indicator and (when the symbol is index 0) text. */
+ text: string;
+ /** Background color for the increase/decrease pill. */
+ background: string;
+ /** Foreground color for the increase/decrease pill's text. */
+ strongText: string;
+}
+
+/**
+ * Resolves the increase/decrease colors to use for rendering, given the
+ * chart's current `increaseColor` / `decreaseColor` (from the
+ * `ColorPickerControl`s added after this became customizable) and the
+ * legacy `comparisonColorScheme` field.
+ *
+ * Charts saved before `increaseColor` / `decreaseColor` existed only have
+ * `comparisonColorScheme`, a 2-choice select ('Green' | 'Red') where 'Green'
+ * meant "green for increase, red for decrease" and 'Red' meant the reverse.
+ * Both legacy choices map onto the same 'Green' | 'Red' semantic token names
+ * used by the new controls' presets, so resolving through it here
+ * reproduces the exact old behavior (including the reversed case) without a
+ * data migration.
+ */
+export const resolveComparisonColorKeys = (
+ comparisonColorScheme: string | undefined,
+ increaseColor: string | undefined,
+ decreaseColor: string | undefined,
+): { increaseColor: string; decreaseColor: string } => {
+ const legacyReversed = comparisonColorScheme === ColorSchemeEnum.Red;
+ return {
+ increaseColor:
+ increaseColor ??
+ (legacyReversed ? ColorSchemeEnum.Red : ColorSchemeEnum.Green),
+ decreaseColor:
+ decreaseColor ??
+ (legacyReversed ? ColorSchemeEnum.Green : ColorSchemeEnum.Red),
+ };
+};
+
+/**
+ * Hex alpha suffix appended to a custom comparison color to build the pill
+ * background tint: 0x1A / 0xFF is roughly 10% opacity.
+ */
+export const COMPARISON_TINT_ALPHA_HEX = '1A';
+
+/**
+ * Resolves a single color value (semantic token name or literal hex from
+ * the color picker) to the (arrow/text, background, strong-text) triad used
+ * across the comparison pills. 'Green' / 'Red' keep using the paired
+ * success/error theme tokens exactly as before these colors were
+ * customizable; any other value is either a theme token name (e.g.
+ * 'colorPrimary', emitted by the picker's `resolveThemeTokens` option) or a
+ * literal hex -- 6-digit, or 8-digit when the alpha-enabled picker is used
+ * -- in which case the background is a light (~10% opacity) tint of that
+ * same color.
+ */
+export const getComparisonColorTokens = (
+ colorValue: string,
+ theme: SupersetTheme,
+): ComparisonColorTokens => {
+ if (colorValue === ColorSchemeEnum.Green) {
+ return {
+ text: theme.colorSuccess,
+ background: theme.colorSuccessBg,
+ strongText: theme.colorSuccessText,
+ };
+ }
+ if (colorValue === ColorSchemeEnum.Red) {
+ return {
+ text: theme.colorError,
+ background: theme.colorErrorBg,
+ strongText: theme.colorErrorText,
+ };
+ }
+ const themeColors = theme as unknown as Record<string, unknown>;
+ const themeValue = Object.prototype.hasOwnProperty.call(
+ themeColors,
+ colorValue,
+ )
+ ? themeColors[colorValue]
+ : undefined;
+ const resolvedColor =
+ typeof themeValue === 'string' ? themeValue : colorValue;
+ // An 8-digit hex (alpha-enabled picker) already carries its own alpha
+ // channel; strip it before appending the tint suffix below so the
+ // background stays a valid 8-digit hex instead of stacking a second one.
+ const isEightDigitHex = /^#[0-9a-f]{8}$/i.test(resolvedColor);
+ const isSixDigitHex = /^#[0-9a-f]{6}$/i.test(resolvedColor);
+ if (!isEightDigitHex && !isSixDigitHex) {
+ // Non-hex theme tokens (e.g. an antd token resolving to an
+ // `rgb(...)`/`rgba(...)` string) can't take a hex alpha suffix without
+ // producing invalid CSS. Rebuild the background with a low alpha from
+ // the color's own r/g/b components instead, so the pill still gets a
+ // light tint rather than becoming the same color as its own text.
+ const rgbMatch = resolvedColor.match(
Review Comment:
`increase_color`/`decrease_color` are also control names in Waterfall and
Candlestick, where the value is an `{r, g, b, a}` object (the Waterfall default
is `{ r: 90, g: 193, b: 137, a: 1 }`). Switching one of those charts to Big
Number Period-over-Period in Explore carries the same-named values across, and
`resolveComparisonColorKeys` keeps them because they are not nullish. When
comparison colors are enabled and the difference is non-zero,
`resolvedColor.match(...)` is called on an object and throws, so the chart
fails to render. Can the helper normalize or ignore non-string values?
##########
superset/mcp_service/chart/preview_utils.py:
##########
@@ -1185,9 +1185,20 @@ def numeric_bound(field_name: str) -> float | None:
}
)
+ explicit_colors = form_data.get("interval_colors")
colors = [
_GAUGE_COLORS[(int(index) - 1) % len(_GAUGE_COLORS)] for index in
color_indices
] or list(_GAUGE_COLORS[: max(1, len(interval_bounds))])
+ if isinstance(explicit_colors, list):
+ # Explicit per-interval colors win; blanks keep the palette fallback.
+ colors = [
+ custom if isinstance(custom, str) and custom else fallback
+ for custom, fallback in zip(
Review Comment:
The explicit colors are zipped against `colors`, whose length comes from the
legacy indices or the 8-entry fallback palette, not from the number of interval
bounds. A gauge with nine bounds and nine `interval_colors` (no legacy indices)
loses the ninth color, and one with `interval_color_indices: "1,2"` and three
explicit colors loses the third. The native chart renders every band, so the
preview ends up with fewer bands than the real chart. Should the result be
sized by `len(interval_bounds)`, with the fallback palette cycled?
##########
superset-frontend/src/explore/store.ts:
##########
@@ -89,6 +105,50 @@ 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.
+ if (
+ 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(
+ bounds,
+ formData.interval_color_indices,
+ formData.color_scheme,
+ );
+ if (resolved.some(color => color !== '')) {
+ formData.interval_colors = resolved;
Review Comment:
This resolves legacy gauge indices against the chart's own `color_scheme`
when the form data is hydrated, and the result is stored as literal
`interval_colors`. On a dashboard with a color scheme override
(`getFormDataWithExtraFilters` sets `color_scheme` from the dashboard after
`applyDefaultFormData` runs), a legacy gauge that used to take its interval
colors from the dashboard palette will now keep the old chart-level palette
colors, since `interval_colors` wins in `getIntervalBoundsAndColors`. Existing
dashboards would change color without anyone editing or saving the chart.
Should the migration be limited to the Explore edit path, or should the legacy
indices stay bound to the effective scheme at render time?
--
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]
