rusackas commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4297914208
##########
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(
Review Comment:
Good catch. The dashboard hydration path (applyDefaultFormData) no longer
resolves the legacy Gauge indices into literal colors, so they stay
palette-relative and the renderer resolves them against the effective scheme
after the dashboard override is applied. The migration still runs when opening
the chart in Explore.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx:
##########
@@ -278,15 +279,26 @@ const config: ControlPanelConfig = {
],
[
{
- name: 'interval_color_indices',
+ name: 'interval_colors',
config: {
- type: 'TextControl',
+ type: 'IntervalColorsControl',
Review Comment:
Fair, the old text input was keyboard friendly and the antd trigger isn't.
ColorPickerControl now makes the swatch a focusable button (tabindex + role)
and Enter/Space open the picker, so this covers the Gauge/Bullet rows and the
other users of the control.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts:
##########
@@ -102,23 +107,40 @@ export default function transformProps(
theme.colorFillSecondary,
theme.colorFill,
];
+ // Custom colors (if any) are authored positionally against the original,
+ // pre-sort `ranges` order via the `range_colors` control -- capture each
+ // range's color here, before the descending sort below reorders them.
const sortedRanges = [...ranges]
- .map((value, i) => ({ value, label: rangeLabels[i] }))
+ .map((value, i) => ({
+ value,
+ label: rangeLabels[i],
+ color: rangeColors?.[i] || undefined,
+ }))
.filter(({ value }) => value > axisMin)
.sort((a, b) => b.value - a.value);
// Each band's label sits inside its own top-right corner: the visible strip
// of a nested band ends at its threshold, so the label lands inside the
// range it names and the rightmost label cannot clip at the chart edge.
- const bands = sortedRanges.map(({ value, label }, i) => ({
+ const getBandLabelColor = (fill?: string): string => {
+ const luminance = fill ? getColorLuminance(fill) : null;
+ if (luminance === null) return theme.colorTextSecondary;
+ return luminance < 0.4 ? theme.colorTextLightSolid : theme.colorText;
Review Comment:
Agreed, a theme token can't be right here. The label color now comes from
the fill itself (fixed light or dark foreground by luminance), so white in a
dark theme gets dark text. Near-opaque fills like #000000fc are measured by
their own channels, and only mostly transparent fills (alpha under 0.5) fall
back to the theme secondary text 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',
Review Comment:
Good catch. I kept the control names so saved Big Number charts don't need a
migration, and made the Waterfall and Candlestick readers tolerate a hex string
(falling back to their defaults for anything unrecognized) via a small
normalizeRgbaColor helper, so switching viz types no longer throws.
--
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]
