codeant-ai-for-open-source[bot] commented on code in PR #42053:
URL: https://github.com/apache/superset/pull/42053#discussion_r3696887765
##########
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:
Yes—replace the existing alpha rather than append another one. The gradient
opacity is the effective alpha for the formatter, so the picker’s persisted
alpha should not produce a second channel.
For example, normalize the base color to RGB before calling `addAlpha`:
```typescript
const baseHexColor = rgbaToHex({ ...colorScheme, a: 1 });
```
Then the gradient path emits exactly one alpha channel:
```typescript
return addAlpha(
baseHexColor,
getOpacity(value, cutoffValue, extremeValue, minOpacity, maxOpacity),
);
```
Alternatively, explicitly compose the two alpha values if picker opacity is
intended to affect the gradient:
```typescript
const effectiveAlpha = pickerAlpha * gradientOpacity;
return rgbaToHex({ ...colorScheme, a: effectiveAlpha });
```
Given the existing gradient behavior, replacing the persisted alpha with the
calculated gradient opacity is the minimal and least surprising fix. Add a
regression test for an RGBA input such as `{ r: 255, g: 0, b: 0, a: 0.5 }` and
assert that the result is an 8-digit hex value, never a 10-digit value.
##########
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:
Yes. Token remapping should be opt-in for conditional formatting, while the
default `ColorPickerControl` behavior should preserve its existing `RGBColor`
callback contract.
Add a prop such as `resolveThemeTokens?: boolean` (defaulting to `false`),
and guard both special-color and reverse theme-token conversion:
```tsx
export interface ColorPickerControlProps {
onChange?: (color: ColorPickerValue) => void;
value?: ColorPickerValue;
resolveThemeTokens?: boolean;
// ...
}
export default function ColorPickerControl({
onChange,
value,
resolveThemeTokens = false,
// ...
}: ColorPickerControlProps) {
// ...
const specialEntry = resolveThemeTokens
? Object.entries(SPECIAL_COLORS).find(
([, rgba]) => rgbaToHex(rgba).toLowerCase() === hex,
)
: undefined;
if (specialEntry) {
onChange(specialEntry[0] as SpecialColorKey);
return;
}
if (resolveThemeTokens && reverseMap[hex]) {
onChange(reverseMap[hex]);
return;
}
onChange(rgb);
}
```
Enable it only in `FormattingPopoverContent`:
```tsx
<ColorPickerControl
ariaLabel={t('Color scheme')}
onChange={event => handleChange(event)}
presets={[...colors, ...extraColorChoices]}
resolveThemeTokens
/>
```
This keeps conditional-formatting values compatible with theme switching,
while contour and breakpoint controls continue receiving RGB objects and pass
their existing validation. Regression tests should verify both modes.
--
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]
