sadpandajoe commented on code in PR #33247:
URL: https://github.com/apache/superset/pull/33247#discussion_r2801518136
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Geojson/Geojson.tsx:
##########
@@ -321,7 +321,9 @@ export const getLayer: GetLayerType<GeoJsonLayer> =
function ({
getFillColor(feature, filterState?.value),
getLineColor,
getLineWidth: fd.line_width || 1,
- pointRadiusScale: fd.point_radius_scale,
+ getPointRadius: fd.point_radius ?? 10,
+ pointRadiusUnits: fd.point_radius_units ?? 'pixels',
Review Comment:
**[major] Backward-compatibility break for existing charts**
These fallback defaults differ from deck.gl's built-in defaults, which
changes rendering for every existing GeoJSON chart that never stored these new
fields:
| Property | deck.gl default | Fallback here | Impact |
|----------|----------------|---------------|--------|
| `getPointRadius` | `1` | `10` | 10x larger base radius |
| `pointRadiusUnits` | `'meters'` | `'pixels'` | Completely different sizing
model |
**Scenario**: An existing chart saved `point_radius_scale = 200` (from the
old `[0, 100, 200, 300, 500]` choices) but has no `point_radius` or
`point_radius_units` (since those controls didn't exist yet).
- **Before this PR**: `getPointRadius=1 (deck.gl default), units='meters',
scale=200` → 200-meter circles
- **After this PR**: `getPointRadius=10, units='pixels', scale=200` →
2000-pixel circles
**Suggested fix** — use deck.gl's defaults as the fallbacks here, and let
the *control panel* defaults handle the improved UX for *new* charts:
```typescript
getPointRadius: fd.point_radius ?? 1,
pointRadiusUnits: fd.point_radius_units ?? 'meters',
pointRadiusScale: fd.point_radius_scale ?? 1,
```
New charts will still get `point_radius=10, units='pixels', scale=1` from
the control panel defaults. Existing charts without these fields get safe
deck.gl defaults.
**Suggested test** to guard against this regression:
```typescript
describe('getLayer backward compatibility', () => {
it('preserves rendering for existing charts without new point radius
fields', () => {
// Simulate form data from an existing chart that only has
point_radius_scale
const legacyFormData = {
...baseFormData,
point_radius_scale: 200,
// point_radius and point_radius_units intentionally absent
};
const layer = getLayer({ formData: legacyFormData, ...otherArgs });
const props = layer.props;
// Should match deck.gl defaults, NOT the new control panel defaults
expect(props.getPointRadius).toBe(1); // deck.gl default, not
10
expect(props.pointRadiusUnits).toBe('meters'); // deck.gl default, not
'pixels'
expect(props.pointRadiusScale).toBe(200); // user's saved value
preserved
});
it('uses control panel defaults for new charts', () => {
const newChartFormData = {
...baseFormData,
point_radius: 10,
point_radius_units: 'pixels',
point_radius_scale: 1,
};
const layer = getLayer({ formData: newChartFormData, ...otherArgs });
const props = layer.props;
expect(props.getPointRadius).toBe(10);
expect(props.pointRadiusUnits).toBe('pixels');
expect(props.pointRadiusScale).toBe(1);
});
});
```
--
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]
