rusackas commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4309432719
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts:
##########
@@ -102,23 +111,42 @@ 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;
+ // Fixed light/dark foregrounds, not `theme.colorText`: that token flips
+ // to a light color in dark themes and would vanish on a light custom fill.
+ return luminance < 0.4 ? LIGHT_LABEL_COLOR : DARK_LABEL_COLOR;
Review Comment:
Yeah, 0.4 was just a number I picked by eye, not derived from anything real.
Swapped it for an actual WCAG contrast comparison between the fill and each of
the two fixed foregrounds, so it picks whichever one really contrasts better
instead of a cutoff that does not match where they actually break even. Added a
test with #999999 since that'\''s right in the range the old cutoff got wrong.
##########
superset-frontend/src/explore/store.test.tsx:
##########
@@ -373,3 +385,108 @@ test('handleDeprecatedControls is idempotent — no-op when
matrixify_enable alr
expect(formData.matrixify_mode_rows).toBe('dimensions');
expect(formData.matrixify_mode_columns).toBe('metrics');
});
+
+// ============================================================
+// #42910: legacy color-picker field migrations. `comparison_color_scheme`
+// and `interval_color_indices` are no longer registered controls, so
+// `getFormDataFromControls` drops them on the next save — these migrations
+// carry their value forward into the controls that replaced them before
+// that can happen.
+// ============================================================
+
+test('handleDeprecatedControls migrates comparison_color_scheme Green into
increase/decrease colors', () => {
+ const formData: any = {
+ viz_type: VizType.BigNumberPeriodOverPeriod,
+ comparison_color_scheme: 'Green',
+ };
+ handleDeprecatedControls(formData);
+
+ expect(formData.increase_color).toBe('Green');
+ expect(formData.decrease_color).toBe('Red');
+});
+
+test('handleDeprecatedControls migrates the reversed comparison_color_scheme
Red choice', () => {
+ const formData: any = {
+ viz_type: VizType.BigNumberPeriodOverPeriod,
+ comparison_color_scheme: 'Red',
+ };
+ handleDeprecatedControls(formData);
+
+ expect(formData.increase_color).toBe('Red');
+ expect(formData.decrease_color).toBe('Green');
+});
+
+test('handleDeprecatedControls does not override already-set increase/decrease
colors', () => {
+ const formData: any = {
+ viz_type: VizType.BigNumberPeriodOverPeriod,
+ comparison_color_scheme: 'Red',
+ increase_color: '#336699',
+ };
+ handleDeprecatedControls(formData);
+
+ expect(formData.increase_color).toBe('#336699');
+ expect(formData.decrease_color).toBeUndefined();
+});
+
+test('handleDeprecatedControls is a no-op for BigNumberPeriodOverPeriod
without a legacy scheme', () => {
+ const formData: any = { viz_type: VizType.BigNumberPeriodOverPeriod };
+ handleDeprecatedControls(formData);
+
+ expect(formData.increase_color).toBeUndefined();
+ expect(formData.decrease_color).toBeUndefined();
+});
+
+test('handleDeprecatedControls migrates interval_color_indices into
interval_colors', () => {
+ const formData: any = {
+ viz_type: VizType.Gauge,
+ intervals: '20,40,60',
+ interval_color_indices: '2,3',
+ color_scheme: 'testScheme',
+ };
+ handleDeprecatedControls(formData);
+
+ // The uncovered third bound keeps its positional scheme color, matching
+ // what the Gauge renderer drew before the migration.
+ expect(formData.interval_colors).toEqual(['#ff7f0e', '#2ca02c', '#2ca02c']);
+});
+
+test('applyDefaultFormData keeps legacy interval_color_indices unresolved for
dashboards', () => {
+ // The dashboard's color_scheme override is applied after hydration, so the
+ // legacy palette-relative indices must not be frozen into literal colors.
+ const result = applyDefaultFormData({
+ viz_type: VizType.Gauge,
+ datasource: '1__table',
+ intervals: '20,40,60',
+ interval_color_indices: '2,3',
+ color_scheme: 'testScheme',
+ } as any);
+
+ expect(result.interval_color_indices).toBe('2,3');
+ expect(result.interval_colors).toEqual([]);
Review Comment:
This one'\''s actually already fixed on master, a follow-up commit added the
`?? []` guard so it is not failing in CI anymore. But you'\''re right that it
was never really exercising the real default, just papering over
`interval_colors` being absent from `controlsState` entirely. Registered the
actual Gauge `interval_colors` control for that test now so it goes through
`getAllControlsState`'\''s real default instead.
--
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]
