sadpandajoe commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4301743887
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/utils.ts:
##########
@@ -21,17 +21,98 @@
export function tokenizeToNumericArray(value?: string): number[] | null {
if (!value?.trim()) return null;
// Lenient by design: this runs on every keystroke, so partial input like
- // "50," must not throw. Empty and non-numeric tokens are dropped.
+ // "50," must not throw. Empty, non-numeric, and non-finite tokens (e.g.
+ // "Infinity", or a value large enough to overflow to it) are dropped.
+ // `BulletRangeColorsControl` shares this exact function (not a
+ // reimplementation) since its per-range colors are matched positionally
+ // against this same tokenized list -- a divergent tokenizer would
+ // silently misalign colors to the wrong bands.
const numbers = value
.split(',')
.map(token => token.trim())
.filter(token => token !== '')
.map(token => Number(token))
- .filter(n => !Number.isNaN(n));
+ .filter(n => Number.isFinite(n));
return numbers.length ? numbers : null;
}
export function tokenizeToStringArray(value?: string): string[] | null {
if (!value?.trim()) return null;
return value.split(',').map(token => token.trim());
}
+
+/**
+ * Whether every comma-separated token in `value` is itself a complete,
+ * valid number -- i.e. the string isn't in a transient, mid-edit state such
+ * as "20,,60" (a blank token between two valid ones). A single blank
+ * trailing token (e.g. "20,40,") is tolerated, since that's the normal shape
+ * of the string while the user is still typing the next value.
+ *
+ * `tokenizeToNumericArray` is deliberately lenient (it silently drops blank
+ * tokens so partial input never throws), which is right for parsing the
+ * list once it's complete but wrong for anything that positionally matches
+ * against it, like `BulletRangeColorsControl`'s per-range colors -- a blank
+ * in the middle must not be treated as "one fewer range" there, since a
+ * color edit made while the list is in that transient state would
+ * permanently misalign once the blank is filled back in.
+ */
+export function isRangesInputComplete(value?: string): boolean {
+ if (!value?.trim()) return true;
+ const tokens = value.split(',').map(token => token.trim());
+ const tokensToValidate =
+ tokens[tokens.length - 1] === '' ? tokens.slice(0, -1) : tokens;
Review Comment:
A trailing blank is still treated as complete here, so a color edit made
while the ranges text is temporarily `20,40,` passes the guard. With ranges
`20,40,60` and three custom colors, erase the `60` and change the first color:
`rangeValues` has two entries, so `replaceColorAtIndex` writes a two-element
array and the third color is gone for good. Re-entering `60` falls back to the
default band. `IntervalColorsControl` has the same shape. Could the editors
keep the trailing entries (or block edits) while the last token is blank?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/controlPanel.tsx:
##########
@@ -59,6 +62,28 @@ const config: ControlPanelConfig = {
},
},
],
+ [
+ {
+ name: 'range_colors',
Review Comment:
Saving a Bullet with `ranges: "50,100"` and `range_colors: ["#ff0000",
"#00ff00"]` renders red and green bands in Explore, but the MCP Vega-Lite
preview (`_generate_bullet_vega_lite_preview` in
`superset/mcp_service/chart/preview_utils.py`) still colors bands through
`legend_color()` with the default categorical scale and a fixed opacity, and
never reads `range_colors`. The preview then shows different colors than the
saved chart. The Gauge preview was updated for its new colors in this PR, but
Bullet's wasn't. Should the Bullet preview honor `range_colors` the same way?
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts:
##########
@@ -100,21 +125,24 @@ const config: ControlPanelConfig = {
],
[
{
- name: 'comparison_color_scheme',
+ name: 'increase_color',
config: {
- type: 'SelectControl',
- label: t('color scheme for comparison'),
- default: ColorSchemeEnum.Green,
- renderTrigger: true,
- choices: [
- [ColorSchemeEnum.Green, 'Green for increase, red for
decrease'],
- [ColorSchemeEnum.Red, 'Red for increase, green for decrease'],
- ],
- visibility: ({ controls }) =>
- controls?.comparison_color_enabled?.value === true,
+ ...comparisonColorControlConfig,
+ label: t('Color for increase'),
Review Comment:
These two color pickers get no `ariaLabel`, and the picker doesn't derive
one from `label`, so a screen-reader user hears two identically named color
buttons with no way to tell increase from decrease. The `SelectControl` they
replace was labelled by its header. Could each config pass an `ariaLabel` (e.g.
the same text as its `label`)?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts:
##########
@@ -53,29 +53,52 @@ import { getColtypesMapping } from '../utils/series';
export const getIntervalBoundsAndColors = (
intervals: string,
intervalColorIndices: string,
+ intervalColors: string[] | undefined,
colorFn: CategoricalColorScale,
min: number,
max: number,
): Array<[number, string]> => {
let intervalBoundsNonNormalized;
- let intervalColorIndicesArray;
try {
intervalBoundsNonNormalized = parseNumbersList(intervals, ',');
- intervalColorIndicesArray = parseNumbersList(intervalColorIndices, ',');
} catch (error) {
intervalBoundsNonNormalized = [] as number[];
- intervalColorIndicesArray = [] as number[];
}
const intervalBounds = intervalBoundsNonNormalized.map(
bound => (bound - min) / (max - min),
);
- const intervalColors = intervalColorIndicesArray.map(
+
+ // New-style: `interval_colors` holds real hex/rgb colors chosen through
+ // the Gauge control panel's per-interval `IntervalColorsControl`,
+ // positionally matched to the bounds parsed from `intervals`. Prefer this
+ // whenever it's populated.
+ if (Array.isArray(intervalColors) && intervalColors.length > 0) {
+ return intervalBounds.map((val, idx) => [
+ val,
+ intervalColors[idx] || colorFn.colors[idx],
Review Comment:
With one saved custom color and nine or more bounds, the tenth band here
resolves to `intervalColors[9] || colorFn.colors[9]`, which is `undefined` for
a nine-color scheme, while `IntervalColorsControl` displays the wrapped scheme
color (`index % schemeColors.length`). Example: `interval_colors: ["#ff0000"]`,
bounds `10,20,...,100`, `echarts5Colors`: the editor shows `#5470C6` for the
tenth row, and editing any other row persists that color into the untouched
band, which the renderer had drawn without one. Should the renderer wrap with
the same modulo so the two agree?
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts:
##########
@@ -35,6 +36,30 @@ import {
metricNameFontSizeWithVisibility,
} from '../sharedControls';
+// Shared base for the `increase_color` / `decrease_color` pickers so the
+// presets, token resolution, output format and visibility rule can't drift
+// apart. No static default: charts saved before these controls existed only
+// have `comparison_color_scheme` ('Green' | 'Red', where 'Red' reverses
+// increase/decrease colors). Leaving each control's value undefined lets
+// `resolveComparisonColorKeys` (see BigNumberPeriodOverPeriod/utils.ts)
+// resolve the correct color from that legacy scheme at render time. A
+// hardcoded default here would win over the legacy fallback via
+// `applyDefaultFormData` and silently repaint old dashboards.
+const comparisonColorControlConfig: CustomControlItem['config'] = {
+ type: 'ColorPickerControl',
+ renderTrigger: true,
+ presets: [
+ {
+ label: t('Semantic colors'),
+ colors: [ColorSchemeEnum.Green, ColorSchemeEnum.Red],
Review Comment:
Picking the Green preset stores `"Green"`, which `PopKPI` renders with
`theme.colorSuccess`, but `ColorPickerControl` displays it from
`SPECIAL_COLORS` as `#00960033`. With `colorSuccess` set to `#52c41a`, the
swatch shows a dark translucent green, and adjusting only the opacity starts
from that color and writes a literal hex that changes both hue and the semantic
theme link. Should the preset display resolve through the same theme token the
chart uses, or should these presets resolve to real colors when selected?
--
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]
