sadpandajoe commented on code in PR #42053:
URL: https://github.com/apache/superset/pull/42053#discussion_r3696886905
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts:
##########
@@ -270,19 +274,35 @@ export const getColorFunction = (
if (compareResult === false) return undefined;
const { cutoffValue, extremeValue } = compareResult;
+ if (typeof colorScheme === 'string') {
+ if (isSpecialColor(colorScheme)) {
+ return colorScheme;
+ }
+ if (useGradient === false) {
+ return colorScheme;
+ }
+ if (alpha === undefined || alpha) {
+ return addAlpha(
+ colorScheme,
+ getOpacity(value, cutoffValue, extremeValue, minOpacity, maxOpacity),
+ );
+ }
+ return colorScheme;
+ }
+ const baseHexColor = rgbaToHex(colorScheme);
// If useGradient is explicitly false, return solid color
if (useGradient === false) {
- return colorScheme;
+ return baseHexColor;
}
// Otherwise apply gradient (default behavior for backward compatibility)
if (alpha === undefined || alpha) {
return addAlpha(
- colorScheme,
+ baseHexColor,
getOpacity(value, cutoffValue, extremeValue, minOpacity, maxOpacity),
Review Comment:
Agreed—the picker can persist an alpha-bearing RGBA value, and the gradient
path then appends a second alpha byte, producing invalid CSS and dropping the
conditional formatting. Should the formatter replace or explicitly compose the
existing alpha before emitting one valid color?
##########
superset-frontend/src/explore/components/controls/ColorPickerControl.tsx:
##########
@@ -16,70 +16,151 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { getCategoricalSchemeRegistry } from '@superset-ui/core';
+import { useMemo } from 'react';
+import { getCategoricalSchemeRegistry, rgbaToHex } from '@superset-ui/core';
import {
ColorPicker,
type RGBColor,
type ColorValue,
} from '@superset-ui/core/components';
import ControlHeader from '../ControlHeader';
+import { useTheme } from '@apache-superset/core/theme';
+
+const SPECIAL_COLORS = {
+ Red: { r: 150, g: 0, b: 0, a: 0.2 },
+ Green: { r: 0, g: 150, b: 0, a: 0.2 },
+} as const;
+
+type SpecialColorKey = keyof typeof SPECIAL_COLORS;
+export type ColorPickerValue = RGBColor | SpecialColorKey | string;
export interface ColorPickerControlProps {
- onChange?: (color: RGBColor) => void;
- value?: RGBColor;
+ onChange?: (color: ColorPickerValue) => void;
+ value?: ColorPickerValue;
name?: string;
label?: string;
description?: string;
renderTrigger?: boolean;
hovered?: boolean;
warning?: string;
+ presets?: { label: string; colors: string[] }[];
+ ariaLabel?: string;
}
-function rgbToHex(rgb: RGBColor): string {
- const { r, g, b, a = 1 } = rgb;
- const toHex = (value: number) => {
- const hex = Math.round(value).toString(16);
- return hex.length === 1 ? `0${hex}` : hex;
- };
+const getReverseThemeColorMap = (
+ themeColors: Record<string, any>,
+): Record<string, string> => {
+ const reverseMap: Record<string, string> = {};
+ if (!themeColors) return reverseMap;
+
+ Object.entries(themeColors).forEach(([name, value]) => {
+ if (typeof value === 'string') {
+ reverseMap[value.toLowerCase()] = name;
+ }
+ });
+
+ return reverseMap;
+};
- const hexColor = `#${toHex(r)}${toHex(g)}${toHex(b)}`;
+function toDisplayHex(
+ value: ColorPickerValue | undefined,
+ themeColors: Record<string, string>,
+): string | undefined {
+ if (!value) return undefined;
- if (a !== undefined && a !== 1) {
- return `${hexColor}${toHex(Math.round(a * 255))}`;
+ if (typeof value === 'string') {
+ if (value in SPECIAL_COLORS) {
+ return rgbaToHex(SPECIAL_COLORS[value as SpecialColorKey]).toLowerCase();
+ }
+ if (themeColors && value in themeColors) {
+ return themeColors[value].toLowerCase();
+ }
+ return value.toLowerCase();
}
- return hexColor;
+ return rgbaToHex(value).toLowerCase();
}
export default function ColorPickerControl({
onChange,
value,
+ presets: customPresets,
+ ariaLabel,
...headerProps
}: ColorPickerControlProps) {
const categoricalScheme = getCategoricalSchemeRegistry().get();
- const presetColors = categoricalScheme?.colors.slice(0, 9) || [];
+ const defaultPresets = categoricalScheme?.colors.slice(0, 9) || [];
+ const theme = useTheme();
+
+ const themeColors = useMemo<Record<string, string>>(
+ () => (theme as any)?.colors || theme || {},
+ [theme],
+ );
+
+ const reverseMap = useMemo(
+ () => getReverseThemeColorMap(themeColors),
+ [themeColors],
+ );
+
+ const presets = useMemo(() => {
+ if (customPresets) {
+ return customPresets.map(item => ({
+ label: item.label,
+ colors: item.colors.map(color => {
+ if (color in SPECIAL_COLORS) {
+ return rgbaToHex(
+ SPECIAL_COLORS[color as SpecialColorKey],
+ ).toLowerCase();
+ }
+ if (themeColors && color in themeColors) {
+ return themeColors[color].toLowerCase();
+ }
+ return String(color).toLowerCase();
+ }),
+ }));
+ }
+
+ return [
+ {
+ label: 'Theme colors',
+ colors: defaultPresets.map(c => String(c).toLowerCase()),
+ },
+ ];
+ }, [customPresets, themeColors, defaultPresets]);
const handleChange = (color: ColorValue) => {
- if (onChange) {
- const rgb = color.toRgb();
- onChange({
- r: rgb.r,
- g: rgb.g,
- b: rgb.b,
- a: rgb.a,
- });
+ if (!onChange) return;
+
+ const rgb = color.toRgb();
+ const hex = rgbaToHex(rgb).toLowerCase();
+
+ const specialEntry = Object.entries(SPECIAL_COLORS).find(
+ ([, rgba]) => rgbaToHex(rgba).toLowerCase() === hex,
+ );
+
+ if (specialEntry) {
+ onChange(specialEntry[0] as SpecialColorKey);
+ return;
}
+
+ if (reverseMap[hex]) {
+ onChange(reverseMap[hex]);
+ return;
+ }
+
+ onChange(rgb);
Review Comment:
Agreed—the shared picker can now emit a theme-token string to existing
contour and breakpoint callers that still require an RGB object, so their
validation fails and the selected color is lost. Should token remapping be
opt-in for conditional formatting while the default callback preserves the RGB
contract?
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.test.tsx:
##########
@@ -117,20 +114,31 @@ test('renders the correct input fields based on the
selected operator', async ()
});
test('renders None for operator when Green for increase is selected', async ()
=> {
- render(
+ const { container } = render(
<FormattingPopoverContent
onChange={mockOnChange}
columns={columns}
extraColorChoices={extraColorChoices}
/>,
);
- // Select the 'Green for increase' color scheme
- fireEvent.change(screen.getAllByLabelText(/color scheme/i)[0], {
- target: { value: ColorSchemeEnum.Green },
+ const colorPickerTrigger = container.querySelector(
+ '.ant-color-picker-trigger',
+ );
+ expect(colorPickerTrigger).toBeInTheDocument();
+ await userEvent.click(colorPickerTrigger!);
+
+ await waitFor(() => {
+ expect(
+ document.querySelector('.ant-color-picker-presets-items'),
+ ).toBeInTheDocument();
});
- fireEvent.click(await screen.findByTitle(/green for increase/i));
+ const presets = document.querySelectorAll('.ant-color-picker-presets-color');
+ const greenPreset = presets[0];
Review Comment:
Agreed—this clicks the first default theme swatch rather than the Green
trend preset, so the assertion can pass without exercising the Green-to-None
behavior. Should the test select the trend preset explicitly and assert the
resulting operator state?
--
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]
