Copilot commented on code in PR #39926:
URL: https://github.com/apache/superset/pull/39926#discussion_r3236386602
##########
superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.ts:
##########
@@ -150,7 +150,11 @@ function WorldMap(element: HTMLElement, props:
WorldMapProps): void {
fillColor: colorFn(d.name, sliceId),
}));
} else {
- const rawExtents = d3Extent(filteredData, d => d.m1);
+ // Exclude null/zero so they render as "no data" instead of getting a
scale color.
+ const colorableData = filteredData.filter(
+ d => d.m1 != null && d.m1 !== 0,
+ );
+ const rawExtents = d3Extent(colorableData, d => d.m1);
Review Comment:
After this change, `processedData` is built from `colorableData`, but
`radiusScale` is still derived earlier from `filteredData`. This can skew
bubble sizes for the remaining displayed countries if the excluded rows had
large/small `m2` values, because the scale’s domain will include data that is
no longer rendered. Recommend computing the radius extent/scale from the same
dataset that is actually rendered in this branch (e.g., `colorableData` when
`colorBy === Metric`), or otherwise ensuring the radius scale domain matches
`processedData`.
##########
superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts:
##########
@@ -511,25 +511,66 @@ test('assigns fill colors from sequential scheme when
colorBy is metric', () =>
expect(data.CAN.fillColor).toMatch(/^(#|rgb)/);
});
-test('falls back to theme.colorBorder when metric values are null', () => {
+test('excludes countries with null metric value from data when colorBy is
metric', () => {
WorldMap(container, {
...baseProps,
colorBy: ColorBy.Metric,
data: [
{
country: 'USA',
name: 'United States',
+ m1: 100,
+ m2: 200,
+ code: 'US',
+ latitude: 37.0902,
+ longitude: -95.7129,
+ },
+ {
+ country: 'CAN',
+ name: 'Canada',
m1: null as unknown as number,
Review Comment:
Using `null as unknown as number` defeats type safety and is a sign that the
runtime shape (nullable `m1`) isn’t represented in the TypeScript types. Prefer
updating the relevant data type(s) to allow `m1: number | null` (or making the
test fixture `data` typed as `any/unknown` at the object/array boundary) so
tests don’t require unsafe per-field casts.
--
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]
