rusackas commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4309435948
##########
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:
Confirmed, erasing the last value and picking a color right then was writing
a shorter array and dropping the last color for good. Both controls now keep
the array at its previous length while there'\''s a trailing blank token, the
same window isRangesInputComplete already tolerates, instead of shrinking to
the transiently-shorter parsed list. Added a test on each control that erases
the last value, picks a color on row 0, and checks the third color survives.
##########
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:
Good catch, the new-style fallback was indexing colorFn.colors straight off
the end instead of wrapping like IntervalColorsControl'\''s own display does.
Wrapped it with the same modulo so the two agree, and editing an untouched band
can'\''t persist undefined into it anymore.
##########
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:
Good catch, added ariaLabel to both.
--
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]
