Copilot commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r3887611234
##########
superset-frontend/plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx:
##########
@@ -278,15 +279,22 @@ const config: ControlPanelConfig = {
],
[
{
- name: 'interval_color_indices',
+ name: 'interval_colors',
config: {
- type: 'TextControl',
+ type: 'IntervalColorsControl',
label: t('Interval colors'),
description: t(
- 'Comma-separated color picks for the intervals, e.g. 1,2,4.
Integers denote colors from the chosen color scheme and are 1-indexed. Length
must be matching that of interval bounds.',
+ 'Pick a color for each interval band defined above by its
upper bound. Charts saved with the legacy 1-indexed "Interval colors" text
field are automatically resolved against the chosen color scheme the first time
this panel is opened.',
),
renderTrigger: true,
- default: DEFAULT_FORM_DATA.intervalColorIndices,
+ default: DEFAULT_FORM_DATA.intervalColors,
+ shouldMapStateToProps: () => true,
+ mapStateToProps: (state: ControlPanelState) => ({
+ intervals: state?.controls?.intervals?.value as string,
+ legacyIntervalColorIndices: state?.controls
+ ?.interval_color_indices?.value as string,
Review Comment:
`interval_color_indices` is no longer part of this control-panel config, so
`getAllControlsState` never creates `state.controls.interval_color_indices`.
This lookup is therefore always undefined for a legacy chart. Explore also
builds its render/save form data from the configured controls only, so opening
a chart saved with non-sequential legacy indices (for example `2,3`)
immediately previews and can save sequential scheme colors instead. Preserve
the legacy field as a hidden control or migrate its value into
`interval_colors` during control initialization.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx:
##########
@@ -278,15 +279,22 @@ const config: ControlPanelConfig = {
],
[
{
- name: 'interval_color_indices',
+ name: 'interval_colors',
config: {
- type: 'TextControl',
+ type: 'IntervalColorsControl',
Review Comment:
`IntervalColorsControl` is registered in the runtime control map, but it is
not included in `InternalControlType`
(`packages/superset-ui-chart-controls/src/types.ts:162-196`). Consequently this
string is not assignable to `ControlPanelConfig`'s `ControlType`, so the
frontend type check will fail. Add the new control name to that union as part
of this change.
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts:
##########
@@ -74,3 +76,75 @@ export const getHeaderFontSize = (proportionValue: number) =>
export const getComparisonFontSize = (proportionValue: number) =>
comparisonFontSizesMapping[proportionValue] ??
sharedFontSizes[sharedFontSizes.length - 1];
+
+export interface ComparisonColorTokens {
+ /** Color for the arrow indicator and (when the symbol is index 0) text. */
+ text: string;
+ /** Background color for the increase/decrease pill. */
+ background: string;
+ /** Foreground color for the increase/decrease pill's text. */
+ strongText: string;
+}
+
+/**
+ * Resolves the increase/decrease colors to use for rendering, given the
+ * chart's current `increaseColor` / `decreaseColor` (from the
+ * `ColorPickerControl`s added after this became customizable) and the
+ * legacy `comparisonColorScheme` field.
+ *
+ * Charts saved before `increaseColor` / `decreaseColor` existed only have
+ * `comparisonColorScheme`, a 2-choice select ('Green' | 'Red') where 'Green'
+ * meant "green for increase, red for decrease" and 'Red' meant the reverse.
+ * Both legacy choices map onto the same 'Green' | 'Red' semantic token names
+ * used by the new controls' presets, so resolving through it here
+ * reproduces the exact old behavior (including the reversed case) without a
+ * data migration.
+ */
+export const resolveComparisonColorKeys = (
+ comparisonColorScheme: string | undefined,
+ increaseColor: string | undefined,
+ decreaseColor: string | undefined,
+): { increaseColor: string; decreaseColor: string } => {
+ const legacyReversed = comparisonColorScheme === ColorSchemeEnum.Red;
+ return {
+ increaseColor:
+ increaseColor ??
+ (legacyReversed ? ColorSchemeEnum.Red : ColorSchemeEnum.Green),
+ decreaseColor:
+ decreaseColor ??
+ (legacyReversed ? ColorSchemeEnum.Green : ColorSchemeEnum.Red),
+ };
+};
+
+/**
+ * Resolves a single color value (semantic token name or literal hex from
+ * the color picker) to the (arrow/text, background, strong-text) triad used
+ * across the comparison pills. 'Green' / 'Red' keep using the paired
+ * success/error theme tokens exactly as before these colors were
+ * customizable; any other value is a literal hex, in which case the
+ * background is a light (~10% opacity) tint of that same color.
+ */
+export const getComparisonColorTokens = (
+ colorValue: string,
+ theme: SupersetTheme,
+): ComparisonColorTokens => {
+ if (colorValue === ColorSchemeEnum.Green) {
+ return {
+ text: theme.colorSuccess,
+ background: theme.colorSuccessBg,
+ strongText: theme.colorSuccessText,
+ };
+ }
+ if (colorValue === ColorSchemeEnum.Red) {
+ return {
+ text: theme.colorError,
+ background: theme.colorErrorBg,
+ strongText: theme.colorErrorText,
+ };
+ }
+ return {
+ text: colorValue,
+ background: `${colorValue}1A`,
+ strongText: colorValue,
Review Comment:
The non-semantic branch cannot assume `colorValue` is a six-digit hex value.
With `resolveThemeTokens`, `ColorPickerControl` emits any matching theme token
name (for example `colorPrimary`), and its alpha-enabled picker can emit
eight-digit hex values. Those inputs produce invalid CSS such as
`colorPrimary1A` or `#336699801A`, and token names are also invalid as the
`text` color. Resolve arbitrary token names through `theme` first, then
generate the 10% tint by replacing/normalizing alpha rather than appending it.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/controlPanel.tsx:
##########
@@ -59,6 +62,25 @@ const config: ControlPanelConfig = {
},
},
],
+ [
+ {
+ name: 'range_colors',
+ config: {
+ type: 'BulletRangeColorsControl',
Review Comment:
`BulletRangeColorsControl` is added to the runtime map but not to the
`InternalControlType` union in
`packages/superset-ui-chart-controls/src/types.ts:162-196`. This literal
therefore does not satisfy the typed `ControlPanelConfig` and breaks the
frontend type check. Please add the new control name to that union.
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts:
##########
@@ -100,21 +100,48 @@ const config: ControlPanelConfig = {
],
[
{
- name: 'comparison_color_scheme',
+ name: 'increase_color',
config: {
- type: 'SelectControl',
- label: t('color scheme for comparison'),
+ type: 'ColorPickerControl',
+ label: t('Color for increase'),
default: ColorSchemeEnum.Green,
Review Comment:
This default defeats the legacy fallback at runtime. Dashboard hydration
calls `applyDefaultFormData`, which fills every missing configured control with
its default; an old chart containing only `comparison_color_scheme: 'Red'`
therefore receives `increase_color: 'Green'` (and the sibling decrease default)
before `resolveComparisonColorKeys` runs. Because the new values are then
present, the resolver never applies the reversed legacy scheme, so existing
dashboards change colors. The new controls need to remain absent or be
initialized from the legacy scheme rather than unconditionally defaulted.
--
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]
