floriandeutsch89 commented on PR #42921:
URL: https://github.com/apache/superset/pull/42921#issuecomment-5228542709
> The flagged issue is correct. The cloned `geojsonFillColorPicker` inherits
the default configuration from the shared `fillColorPicker`, which includes a
non-transparent default color. This causes the GeoJSON layer to override
feature-specific colors by default.
>
> To resolve this, you should explicitly override the `default` property in
the `geojsonFillColorPicker` configuration to be transparent (e.g., `{ r: 0, g:
0, b: 0, a: 0 }`).
>
> Here is the corrected implementation for
`superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/controlPanel.ts`:
>
> ```ts
> const geojsonFillColorPicker = {
> ...fillColorPicker,
> config: {
> ...fillColorPicker.config,
> default: { r: 0, g: 0, b: 0, a: 0 },
> visibility: () => true,
> },
> };
> ```
>
> I have checked the PR comments, and there are no other pending review
comments to address.
>
>
**superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/controlPanel.ts**
>
> ```
> const geojsonFillColorPicker = {
> ...fillColorPicker,
> config: {
> ...fillColorPicker.config,
> default: { r: 0, g: 0, b: 0, a: 0 },
> visibility: () => true,
> },
> };
> ```
I don't think this change is right, for three reasons.
**1. It renders GeoJSON charts blank.** In `Shared_DeckGL.tsx`, `filled`
defaults
to `true` and `stroked` defaults to `false`. With `default: { r: 0, g: 0, b:
0, a: 0 }`
the fill is transparent and there is no outline, so any GeoJSON without
per-feature
colour properties — plain geometry, `ST_AsGeoJSON` output, most real-world
data —
renders as a completely empty map. No fill, no stroke, no error, no
indication why.
That is a worse and much harder-to-diagnose default than a visible polygon
the user
can recolour.
**2. It doesn't fix the affected charts.** Control defaults only apply where
`form_data` has no value. Every GeoJSON chart saved through Explore in
6.0/6.1
already has `fill_color_picker: PRIMARY_COLOR` persisted, so those charts
stay teal
after this change. Restoring the visible control — what this PR does — is
the only
thing that lets those users set opacity 0 and recover their feature colours.
**3. It isn't a defect introduced here.** This PR spreads
`fillColorPicker.config`
and overrides only `visibility`; `default` is unchanged from before #34017.
The
override is current behaviour on master, since `Geojson.tsx` falls back
independently
of the control:
const fc = fd.fill_color_picker ?? PRIMARY_COLOR;
So the teal override predates this PR, which restores the only means of
escaping it.
Whether feature-provided colours should win by default is a reasonable UX
question,
but it's a behaviour change affecting new charts, needs `stroked` handled so
charts
don't render blank, and would leave `strokeColorPicker` (same
`PRIMARY_COLOR` default,
ungated) inconsistent unless changed too. I'd rather keep this PR as a scoped
regression fix and raise that separately — happy to open an issue.
--
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]
