rusackas commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r3887770126
##########
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:
Good catch, fixed. Dropped the static default on
`increase_color`/`decrease_color` so the legacy `comparison_color_scheme`
fallback in `resolveComparisonColorKeys` still kicks in instead of getting
preempted by `applyDefaultFormData`.
##########
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:
Fixed. `interval_color_indices` isn't a registered control anymore, so
`mapStateToProps` now reads it off `state.form_data` instead of
`state.controls`, matching the pattern already used for things like
`server_pagination` elsewhere in the codebase.
##########
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:
Fixed. `getComparisonColorTokens` now resolves theme token names via the
theme object and strips any pre-existing alpha channel before appending the
tint suffix, so it stays valid CSS either way.
##########
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` only ever shows up in a `controlSetRows` entry,
which types `config` as `BaseControlConfig<any>` via `CustomControlItem`, not
against `InternalControlType` directly. tsc already runs clean here in
`validate-frontend` on this commit, so there's no type error to add it for.
##########
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:
Same story as the IntervalColorsControl thread on this PR, this goes through
CustomControlItem's config: BaseControlConfig<any>, not InternalControlType, so
it's already unchecked there and tsc is green on this commit.
--
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]
