EnxDev commented on code in PR #37625:
URL: https://github.com/apache/superset/pull/37625#discussion_r2766319396
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.tsx:
##########
@@ -218,11 +272,11 @@ class ScatterPlotGlowOverlay extends PureComponent {
ctx.fillStyle = gradient;
ctx.fill();
- if (Number.isFinite(parseFloat(clusterLabel))) {
- if (clusterLabel >= 10000) {
- clusterLabel = `${Math.round(clusterLabel / 1000)}k`;
- } else if (clusterLabel >= 1000) {
- clusterLabel = `${Math.round(clusterLabel / 100) / 10}k`;
+ if (Number.isFinite(parseFloat(String(clusterLabel)))) {
+ if ((clusterLabel as number) >= 10000) {
+ clusterLabel = `${Math.round((clusterLabel as number) /
1000)}k`;
+ } else if ((clusterLabel as number) >= 1000) {
+ clusterLabel = `${Math.round((clusterLabel as number) / 100) /
10}k`;
Review Comment:
It looks like the same variable is being cast to `number`several times.
Since it’s already validated as numeric, perhaps we could convert it once at
the top?
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.tsx:
##########
@@ -280,29 +343,29 @@ class ScatterPlotGlowOverlay extends PureComponent {
MIN_POINT_RADIUS +
normalizedValue * (MAX_POINT_RADIUS - MIN_POINT_RADIUS);
}
- pointLabel = `${roundDecimal(radiusProperty, 2)}`;
+ pointLabel = `${roundDecimal(radiusProperty as number, 2)}`;
Review Comment:
Do we still need this cast after the check?
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.tsx:
##########
@@ -234,23 +288,32 @@ class ScatterPlotGlowOverlay extends PureComponent {
}
} else {
const defaultRadius = radius / 6;
- const radiusProperty = location.properties.radius;
- const pointMetric = location.properties.metric;
- let pointRadius =
- radiusProperty === null ? defaultRadius : radiusProperty;
- let pointLabel;
-
- if (radiusProperty !== null) {
- const pointLatitude = lngLatAccessor(location)[1];
+ const radiusProperty = location.properties.radius as
+ | number
+ | null
+ | undefined;
+ const pointMetric = location.properties.metric as
+ | number
+ | string
+ | null;
+ // Handle both null and undefined as "missing" - use default radius
+ let pointRadius: number =
+ radiusProperty == null
+ ? defaultRadius
+ : (radiusProperty as number);
Review Comment:
Would this approach make sense to you? It avoids casts, validates at
runtime, and falls back gracefully rather than producing invisible points.
```suggestion
const rawRadius = location.properties.radius;
const radiusProperty = typeof rawRadius === 'number' ? rawRadius
: null;
const pointMetric = location.properties.metric ?? null;
const pointRadius: number = radiusProperty ?? defaultRadius;
```
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.tsx:
##########
@@ -280,29 +343,29 @@ class ScatterPlotGlowOverlay extends PureComponent {
MIN_POINT_RADIUS +
normalizedValue * (MAX_POINT_RADIUS - MIN_POINT_RADIUS);
}
- pointLabel = `${roundDecimal(radiusProperty, 2)}`;
+ pointLabel = `${roundDecimal(radiusProperty as number, 2)}`;
} else if (
Number.isFinite(minRadiusValue) &&
minRadiusValue === maxRadiusValue
) {
// All values are the same, use a fixed medium size
pointRadius = (MIN_POINT_RADIUS + MAX_POINT_RADIUS) / 2;
- pointLabel = `${roundDecimal(radiusProperty, 2)}`;
+ pointLabel = `${roundDecimal(radiusProperty as number, 2)}`;
} else {
// Use raw pixel values if they're already in a reasonable
range
pointRadius = Math.max(
MIN_POINT_RADIUS,
Math.min(pointRadius, MAX_POINT_RADIUS),
);
- pointLabel = `${roundDecimal(radiusProperty, 2)}`;
+ pointLabel = `${roundDecimal(radiusProperty as number, 2)}`;
Review Comment:
same here
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.tsx:
##########
@@ -280,29 +343,29 @@ class ScatterPlotGlowOverlay extends PureComponent {
MIN_POINT_RADIUS +
normalizedValue * (MAX_POINT_RADIUS - MIN_POINT_RADIUS);
}
- pointLabel = `${roundDecimal(radiusProperty, 2)}`;
+ pointLabel = `${roundDecimal(radiusProperty as number, 2)}`;
} else if (
Number.isFinite(minRadiusValue) &&
minRadiusValue === maxRadiusValue
) {
// All values are the same, use a fixed medium size
pointRadius = (MIN_POINT_RADIUS + MAX_POINT_RADIUS) / 2;
- pointLabel = `${roundDecimal(radiusProperty, 2)}`;
+ pointLabel = `${roundDecimal(radiusProperty as number, 2)}`;
Review Comment:
here as well
--
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]
