Copilot commented on code in PR #42053:
URL: https://github.com/apache/superset/pull/42053#discussion_r3607020196
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts:
##########
@@ -270,19 +270,29 @@ export const getColorFunction = (
if (compareResult === false) return undefined;
const { cutoffValue, extremeValue } = compareResult;
+ if (typeof colorScheme === 'string') {
+ if (alpha === undefined || alpha) {
+ return addAlpha(
+ colorScheme,
+ getOpacity(value, cutoffValue, extremeValue, minOpacity, maxOpacity),
Review Comment:
`useGradient === false` is ignored when `colorScheme` is a string, because
the early string-return branch always applies `addAlpha(...)` (when `alpha` is
enabled). This makes the "Use gradient" toggle ineffective for string-based
color schemes (e.g., theme-resolved hex strings).
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -320,6 +322,9 @@ export const FormattingPopoverContent = ({
[allColumns],
);
+ const defaultColorValue =
+ theme[colorScheme()[0]?.colors[0] as keyof typeof theme] || undefined;
Review Comment:
`defaultColorValue` recomputes `colorScheme()` and falls back to `undefined`
if the theme doesn’t contain the expected token key. That can leave a required
field without an initial value. Using the already-computed `colors` and falling
back to the token string is safer.
##########
superset-frontend/src/explore/components/controls/ColorPickerControl.tsx:
##########
@@ -16,51 +16,90 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { getCategoricalSchemeRegistry } from '@superset-ui/core';
+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[] }[];
}
-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;
- };
+function toDisplayHex(
+ value: ColorPickerValue | undefined,
+ theme?: any,
+): string | undefined {
+ if (!value) return undefined;
- const hexColor = `#${toHex(r)}${toHex(g)}${toHex(b)}`;
-
- 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]);
+ }
+ if (theme && value in theme) {
+ return theme[value as keyof typeof theme];
+ }
+ return value;
}
- return hexColor;
+ return rgbaToHex(value);
}
export default function ColorPickerControl({
onChange,
value,
+ presets: customPresets,
...headerProps
}: ColorPickerControlProps) {
const categoricalScheme = getCategoricalSchemeRegistry().get();
- const presetColors = categoricalScheme?.colors.slice(0, 9) || [];
+ const defaultPresets = categoricalScheme?.colors.slice(0, 9) || [];
+ const theme = useTheme();
+ const presets = customPresets
+ ? customPresets.map(item => ({
+ label: item.label,
+ colors: item.colors.map(color => {
+ if (theme && color in theme) {
+ return theme[color as keyof typeof theme];
+ }
+ if (color in SPECIAL_COLORS) {
+ return rgbaToHex(SPECIAL_COLORS[color as SpecialColorKey]);
+ }
+ return color;
+ }),
+ }))
+ : [{ label: 'Theme colors', colors: defaultPresets }];
const handleChange = (color: ColorValue) => {
- if (onChange) {
+ if (!onChange) return;
+
+ const hex = rgbaToHex(color.toRgb());
+
Review Comment:
When selecting a preset theme token color (e.g. `colorSuccess`),
`handleChange` emits an `RGBColor` object, which bakes the current theme’s
resolved hex into the saved config and prevents later theme switching. Since
`customPresets` already contains the original preset strings, map the picked
hex back to the preset value when there’s a match.
##########
superset-frontend/src/explore/components/controls/ColorPickerControl.tsx:
##########
@@ -16,51 +16,90 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { getCategoricalSchemeRegistry } from '@superset-ui/core';
+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[] }[];
}
-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;
- };
+function toDisplayHex(
+ value: ColorPickerValue | undefined,
+ theme?: any,
+): string | undefined {
Review Comment:
`toDisplayHex` takes `theme?: any`, which defeats TypeScript’s safety and
makes it easy to index the theme incorrectly. You can type this without
introducing new imports by using `ReturnType<typeof useTheme>`.
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -395,12 +400,11 @@ export const FormattingPopoverContent = ({
name="colorScheme"
label={t('Color scheme')}
rules={rulesRequired}
- initialValue={colorScheme[0].value}
+ initialValue={defaultColorValue}
>
- <Select
+ <ColorPickerControl
onChange={event => handleChange(event)}
- ariaLabel={t('Color scheme')}
- options={[...colorScheme, ...extraColorChoices]}
+ presets={[...colors, ...extraColorChoices]}
/>
Review Comment:
The color picker trigger rendered inside this `FormItem` no longer has an
accessible name tied to the "Color scheme" label (tests had to switch away from
`getByLabelText`). This is an accessibility regression compared to the previous
Select. Consider forwarding an `aria-label`/`aria-labelledby` to the
ColorPicker trigger so the field remains discoverable to screen readers.
--
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]
