Copilot commented on code in PR #36847:
URL: https://github.com/apache/superset/pull/36847#discussion_r2673599581
##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -300,6 +315,34 @@ export default function transformProps(
},
series,
};
+
+ // this will add visualMap for sequential coloring when appropriate
+ if (useSequential) {
+ const startColor = colorFn('min', sliceId);
+ const endColor = colorFn('max', sliceId);
+ // try to pick a proper sequential palette from registry using the current
colorScheme as a hint
+ const seqScheme = sequentialRegistry?.get?.(
+ (formData as any)?.linearColorScheme || (colorScheme as string),
+ );
+ const inRangeColors: string[] = (
+ seqScheme?.colors && seqScheme.colors.length
+ ? seqScheme.colors
+ : [startColor, endColor]
+ ).filter((c): c is string => !!c);
Review Comment:
Using a categorical color function with 'min' and 'max' as category keys may
not produce appropriate colors for a sequential gradient. If the sequential
scheme is not found in the registry, consider using predefined sequential color
palettes or theme-based sequential colors instead of categorical colors.
```suggestion
// try to pick a proper sequential palette from registry using the
current colorScheme as a hint
const seqScheme = sequentialRegistry?.get?.(
(formData as any)?.linearColorScheme || (colorScheme as string),
);
let inRangeColors: string[] = [];
if (seqScheme?.colors && seqScheme.colors.length) {
inRangeColors = seqScheme.colors.filter((c): c is string => !!c);
} else if (theme) {
// Fall back to theme-based sequential colors instead of categorical
colors
const primary = (theme as any).colorPrimary ?? (theme as
any).colors?.[0];
const secondary = (theme as any).colors?.[1] ?? primary;
if (primary) {
inRangeColors = secondary ? [primary, secondary] : [primary];
}
}
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -205,8 +221,7 @@ export default function transformProps(
item = {
...item,
itemStyle: {
- colorAlpha: OpacityEnum.SemiTransparent,
- color: theme.colorText,
+ opacity: OpacityEnum.SemiTransparent,
borderColor: theme.colorBgBase,
borderWidth: 2,
},
Review Comment:
The itemStyle object is completely replaced when filtering, which discards
the previous properties including gapWidth and color (for categorical mode).
Consider spreading the existing itemStyle properties to preserve them:
`itemStyle: { ...item.itemStyle, opacity: OpacityEnum.SemiTransparent,
borderColor: theme.colorBgBase, borderWidth: 2 }`
##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -300,6 +315,34 @@ export default function transformProps(
},
series,
};
+
+ // this will add visualMap for sequential coloring when appropriate
Review Comment:
The comment says "this will add visualMap for sequential coloring when
appropriate" but would be more accurate as "Add visualMap configuration for
sequential coloring when metric has varying values".
```suggestion
// Add visualMap configuration for sequential coloring when metric has
varying values
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -163,10 +165,24 @@ export default function transformProps(
const metricLabel = getMetricLabel(metric);
const groupbyLabels = groupby.map(getColumnLabel);
const treeData = treeBuilder(data, groupbyLabels, metricLabel);
+ // this will actually prepare sequential color mapping when metric values
are numeric
Review Comment:
The comment says "this will actually prepare sequential color mapping" but
the code only extracts and validates metric values for determining whether to
use sequential coloring. Consider updating the comment to be more precise, such
as "Extract and validate metric values to determine if sequential coloring
should be applied".
```suggestion
// Extract and validate metric values to determine if sequential coloring
should be applied
```
##########
superset-frontend/plugins/plugin-chart-echarts/test/Treemap/transformProps.test.ts:
##########
@@ -74,4 +74,39 @@ describe('Treemap transformProps', () => {
}),
);
});
+
+ it('auto mode: should add visualMap for numeric varying metric', () => {
+ const result = transformProps(chartProps as EchartsTreemapChartProps);
+ expect(result.echartOptions).toHaveProperty('visualMap');
+ // @ts-ignore
+ expect(result.echartOptions.visualMap.min).toBeCloseTo(2.5);
+ // @ts-ignore
+ expect(result.echartOptions.visualMap.max).toBeCloseTo(10);
Review Comment:
The use of @ts-ignore comments suppresses TypeScript type checking. Instead,
properly type the visualMap property or use optional chaining with type
assertions to maintain type safety while accessing these properties.
```suggestion
const visualMap = (result.echartOptions as { visualMap: { min: number;
max: number } }).visualMap;
expect(visualMap.min).toBeCloseTo(2.5);
expect(visualMap.max).toBeCloseTo(10);
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -300,6 +315,34 @@ export default function transformProps(
},
series,
};
+
+ // this will add visualMap for sequential coloring when appropriate
+ if (useSequential) {
+ const startColor = colorFn('min', sliceId);
+ const endColor = colorFn('max', sliceId);
+ // try to pick a proper sequential palette from registry using the current
colorScheme as a hint
+ const seqScheme = sequentialRegistry?.get?.(
+ (formData as any)?.linearColorScheme || (colorScheme as string),
Review Comment:
Using type assertion `formData as any` bypasses type checking. Consider
extending the EchartsTreemapFormData type definition to include
linearColorScheme if it's a valid property, or check if the property exists in
a type-safe manner.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -163,10 +165,24 @@ export default function transformProps(
const metricLabel = getMetricLabel(metric);
const groupbyLabels = groupby.map(getColumnLabel);
const treeData = treeBuilder(data, groupbyLabels, metricLabel);
+ // this will actually prepare sequential color mapping when metric values
are numeric
+ const metricValues = (data || [])
+ .map(row => {
+ const v = row[metricLabel as string];
+ return typeof v === 'number' ? v : Number(v);
+ })
+ .filter(v => Number.isFinite(v));
+ const minMetricValue = metricValues.length ? Math.min(...metricValues) : 0;
+ const maxMetricValue = metricValues.length ? Math.max(...metricValues) : 0;
Review Comment:
Using the spread operator with Math.min and Math.max on potentially large
arrays could cause a stack overflow error if metricValues is very large.
Consider using a reduce-based approach or iterating through the array to find
min/max values for better performance and safety with large datasets.
```suggestion
const { minMetricValue, maxMetricValue } = metricValues.length
? metricValues.reduce(
(acc, v) => ({
minMetricValue: v < acc.minMetricValue ? v : acc.minMetricValue,
maxMetricValue: v > acc.maxMetricValue ? v : acc.maxMetricValue,
}),
{ minMetricValue: metricValues[0], maxMetricValue: metricValues[0] },
)
: { minMetricValue: 0, maxMetricValue: 0 };
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -300,6 +315,34 @@ export default function transformProps(
},
series,
};
+
+ // this will add visualMap for sequential coloring when appropriate
+ if (useSequential) {
+ const startColor = colorFn('min', sliceId);
+ const endColor = colorFn('max', sliceId);
+ // try to pick a proper sequential palette from registry using the current
colorScheme as a hint
+ const seqScheme = sequentialRegistry?.get?.(
+ (formData as any)?.linearColorScheme || (colorScheme as string),
+ );
+ const inRangeColors: string[] = (
+ seqScheme?.colors && seqScheme.colors.length
+ ? seqScheme.colors
+ : [startColor, endColor]
+ ).filter((c): c is string => !!c);
+ if (inRangeColors.length === 0) {
+ // Fall back to a safe theme color if no valid sequential colors are
available
+ inRangeColors.push(theme?.colorPrimary ?? '#000000');
+ }
+ // assign visualMap in a type-safe way
+ (echartOptions as any).visualMap = {
Review Comment:
The comment states "assign visualMap in a type-safe way" but the
implementation uses type assertion with "as any". Consider defining a proper
type that includes the visualMap property or using a more type-safe approach to
extend the echartOptions object.
--
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]
