aminghadersohi commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4429485955
##########
superset-frontend/plugins/plugin-chart-echarts/test/Bullet/transformProps.test.ts:
##########
@@ -439,6 +439,149 @@ test('keeps exact-case dimensions and uses per-index
fallbacks for short labels'
).toEqual(['stretch', '260.0']);
});
+test('applies custom range_colors to their bands, matched by original
(pre-sort) range order', () => {
+ const { echartOptions } = transformProps(
+ chartProps({ rangeColors: ['#ff0000', '#00ff00', '#0000ff'] }),
+ );
+ const { series } = echartOptions as any;
+
+ // bands are drawn largest-threshold-first (300, 200, 100), but colors
+ // must stay pinned to their original range value (100 -> red, 200 ->
+ // green, 300 -> blue), not to the post-sort draw order.
+ const bandColors = series[0].markArea.data.map(
+ (d: any) => d[0].itemStyle.color,
+ );
+ expect(bandColors).toEqual(['#0000ff', '#00ff00', '#ff0000']);
+});
+
+test('picks a contrasting label color for custom range fills', () => {
+ const { echartOptions } = transformProps(
+ chartProps({ showLabels: true, rangeColors: ['#000000', '#ffffff', ''] }),
+ );
+ const labelColors = (echartOptions as any).series[0].markArea.data.map(
+ (d: any) => d[0].label.color,
+ );
+ // bands are drawn largest-threshold-first: unset, white, black
+ expect(labelColors).toEqual([
+ supersetTheme.colorTextSecondary,
+ '#1f1f1f',
+ '#ffffff',
+ ]);
+});
+
+test('uses the nearest outer custom fill to pick an uncolored band label
color', () => {
+ const { echartOptions } = transformProps(
+ chartProps({ showLabels: true, rangeColors: ['', '#000000', ''] }),
+ );
+ const labelColors = (echartOptions as any).series[0].markArea.data.map(
+ (d: any) => d[0].label.color,
+ );
+ // drawn largest-first: unset (outermost), black, unset sitting on black
+ expect(labelColors).toEqual([
+ supersetTheme.colorTextSecondary,
+ '#ffffff',
+ '#ffffff',
+ ]);
+});
Review Comment:
Nit: Reverting the label-fill change to `color ?? outerCustomFill` keeps
every Bullet test green: the only translucent case has no opaque band outside
it. This test pins a translucent band over an opaque one.
```suggestion
});
test('labels a translucent band from the opaque custom fill beneath it', ()
=> {
const { echartOptions } = transformProps(
chartProps({ showLabels: true, rangeColors: ['#ffffff33', '#000000', '']
}),
);
const labelColors = (echartOptions as any).series[0].markArea.data.map(
(d: any) => d[0].label.color,
);
// drawn largest-first: unset, black, translucent white over black
expect(labelColors).toEqual([
supersetTheme.colorTextSecondary,
'#ffffff',
'#ffffff',
]);
});
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts:
##########
@@ -103,23 +130,58 @@ 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],
+ // Colors only apply to user-entered ranges; the automatic fallback band
+ // must not pick up a stale `range_colors` entry kept from earlier edits.
+ color: explicitRanges ? rangeColors?.[i] || undefined : 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. Pick whichever foreground actually contrasts better against
+ // this fill, rather than a single magic luminance cutoff that doesn't
+ // match where white and the dark foreground really break even.
+ return wcagContrast(luminance, BAND_LABEL_LUMINANCE.light) >=
+ wcagContrast(luminance, BAND_LABEL_LUMINANCE.dark)
+ ? BAND_LABEL_COLORS.light
+ : BAND_LABEL_COLORS.dark;
+ };
+ // Bands are drawn outermost first, so a band without its own opaque color
+ // shows the nearest outer opaque custom fill; its label must contrast with
+ // that.
+ let outerCustomFill: string | undefined;
+ const bandLabelFills = sortedRanges.map(({ color }) => {
+ if (color && getColorLuminance(color) !== null) outerCustomFill = color;
+ return outerCustomFill;
+ });
Review Comment:
Nit: My earlier suggestion missed this: `getColorLuminance` is also null for
opaque colors it can't parse, so an API-saved `range_colors` entry like
`yellow` inside a black band now gets white text (~1.07:1). Defer outward only
for fills known to be translucent.
```suggestion
let outerCustomFill: string | undefined;
const bandLabelFills = sortedRanges.map(({ color }) => {
const translucent =
!!color &&
getColorLuminance(color) === null &&
/^(#[0-9a-f]{8}|rgba\()/i.test(color.trim());
if (color && !translucent) outerCustomFill = color;
return outerCustomFill;
});
```
##########
superset-frontend/src/explore/components/controls/ColorPickerControl.test.tsx:
##########
@@ -238,3 +345,96 @@ test('handles undefined theme gracefully', () => {
expect(() => render(<ColorPickerControl {...defaultProps} />)).not.toThrow();
});
+
+test('trigger is exposed as a focusable button for keyboard users', () => {
+ const { container } = render(<ColorPickerControl {...defaultProps} />);
+ const trigger = container.querySelector('.ant-color-picker-trigger');
+ expect(trigger).toHaveAttribute('tabindex', '0');
+ expect(trigger).toHaveAttribute('role', 'button');
+});
+
+test('opens the popup on Enter when the trigger has focus', async () => {
+ render(<ColorPickerControl {...defaultProps} />);
+ const trigger = document.querySelector(
+ '.ant-color-picker-trigger',
+ ) as HTMLElement;
+ trigger.focus();
+ fireEvent.keyDown(trigger, { key: 'Enter' });
+
+ await waitFor(() => {
+ expect(
+ document.querySelector('.ant-color-picker-presets-color'),
+ ).toBeInTheDocument();
+ });
+});
+
+test('opens the popup on Space when the trigger has focus', async () => {
+ render(<ColorPickerControl {...defaultProps} />);
+ const trigger = document.querySelector(
+ '.ant-color-picker-trigger',
+ ) as HTMLElement;
+ trigger.focus();
+ fireEvent.keyDown(trigger, { key: ' ' });
+
+ await waitFor(() => {
+ expect(
+ document.querySelector('.ant-color-picker-presets-color'),
+ ).toBeInTheDocument();
+ });
+});
+
+test('a disabled trigger does not open on Enter or Space', () => {
+ render(<ColorPickerControl {...defaultProps} disabled />);
+ const trigger = document.querySelector(
+ '.ant-color-picker-trigger',
+ ) as HTMLElement;
+ expect(trigger).toHaveAttribute('tabindex', '-1');
+
+ fireEvent.keyDown(trigger, { key: 'Enter' });
+ fireEvent.keyDown(trigger, { key: ' ' });
+
+ expect(
+ document.querySelector('.ant-color-picker-presets-color'),
+ ).not.toBeInTheDocument();
+});
+
+describe('toDisplayHex', () => {
+ test('never resolves theme tokens or SPECIAL_COLORS when resolveThemeTokens
is false', () => {
+ // A raw string that happens to match a numeric antd theme key (e.g.
+ // `fontSize`) must not be looked up -- doing so returns a number, and
+ // calling `.toLowerCase()` on it throws during render.
+ const themeWithNumericToken = { fontSize: 14 } as unknown as Record<
+ string,
+ string
+ >;
+ expect(toDisplayHex('fontSize', themeWithNumericToken, false)).toBe(
+ 'fontsize',
+ );
+
+ // A literal CSS color name that happens to collide with a
+ // SPECIAL_COLORS preset name (e.g. "Red") must render as itself, not
+ // as the muted preset color, when this caller never opted into
+ // SPECIAL_COLORS/theme-token resolution.
+ expect(toDisplayHex('Red', {}, false)).toBe('red');
+ });
+
+ test('resolves SPECIAL_COLORS and theme tokens only when resolveThemeTokens
is true', () => {
+ expect(toDisplayHex('Red', {}, true)).toBe('#96000033');
+ expect(
+ toDisplayHex('colorSuccess', { colorSuccess: '#52C41A' }, true),
+ ).toBe('#52c41a');
+ });
+
+ test('prefers the live theme token over the fixed SPECIAL_COLORS stand-in
for Green/Red', () => {
+ // BigNumberPeriodOverPeriod's increase/decrease pickers render 'Green'
+ // through `theme.colorSuccess` at chart-render time (see
+ // getComparisonColorTokens), so the picker's own preview must show that
+ // same color, not SPECIAL_COLORS' fixed, muted stand-in.
+ expect(toDisplayHex('Green', { colorSuccess: '#52C41A' }, true)).toBe(
+ '#52c41a',
+ );
+ // With no matching theme token available, SPECIAL_COLORS is still the
+ // fallback.
+ expect(toDisplayHex('Green', {}, true)).toBe('#00960033');
+ });
+});
Review Comment:
Nit: Removing either `normalizeOpaqueAlpha` call leaves this suite green, so
the opaque four-digit `colorError` → `Red` fix is unpinned. This test fails
with the shorthand-path call removed.
```suggestion
});
test('stores "Red" when the theme colorError is an opaque four-digit hex',
async () => {
const onChange = jest.fn();
jest
.spyOn(require('@apache-superset/core/theme'), 'useTheme')
.mockReturnValue({ colors: { colorError: '#F00F' } });
render(
<ColorPickerControl
{...defaultProps}
onChange={onChange}
resolveThemeTokens
presets={[{ label: 'Special Colors', colors: ['Green', 'Red'] }]}
/>,
);
await userEvent.click(
document.querySelector('.ant-color-picker-trigger') as HTMLElement,
);
await waitFor(() => {
expect(
document.querySelector('.ant-color-picker-presets-color'),
).toBeInTheDocument();
});
await userEvent.click(
document.querySelectorAll('.ant-color-picker-presets-color')[1],
);
expect(onChange).toHaveBeenCalledWith('Red');
});
```
--
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]
