You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "floriandeutsch89 (via GitHub)" <gi...@apache.org> on 2026/08/08 22:30:18 UTC
[PR] fix(deckgl): restore the Fill Color control on the GeoJSON layer [superset]
floriandeutsch89 opened a new pull request, #42921:
URL: https://github.com/apache/superset/pull/42921
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
Restores the missing Fill Color control on the deck.gl GeoJSON chart, which also fixes the ability to set fill opacity to `0` (falling back to each feature's own`fillColor` property).
Bug: A shared control update (#34017) added a visibility gate (`color_scheme_type`) to `fillColorPicker`. Because the GeoJSON layer didn't adopt `color_scheme_type`, the gate permanently evaluates to `false`, hiding the picker.
-> Fix: Override the visibility gate specifically for the GeoJSON layer's shared control definition rather than adding an unsupported dropdown.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
_Before:_ the **GeoJson Settings** section shows only **Stroke Color**.
_After:_ **Fill Color** appears next to **Stroke Color**, and setting its opacity
to 0 restores the GeoJSON feature's own `fillColor`:
<img width="1661" height="1159" alt="geojson_fix" src="https://github.com/user-attachments/assets/1bfaa37f-281b-4660-90d9-607593108751" />
### TESTING INSTRUCTIONS
1. Open a **deck.gl GeoJSON** where features have built-in `fillColor` properties.
1. Go to **GeoJson Settings** and confirm **Fill Color** is present alongside **Stroke Color**.
1. Test setting opacity > 0 (overrides features) and opacity = 0 (falls back to feature properties).
1. Verify the Polygon chart's Fill Color control still behaves correctly based on its Color scheme type.
Sample dataset SQL (Postgres):
```sql
SELECT * FROM (VALUES
('red'::text,
'{"type":"Feature","properties":{"name":"red","fillColor":"#e04836","strokeColor":"#7d1a0d"},"geometry":{"type":"Polygon","coordinates":[[[13.36,52.50],[13.38,52.50],[13.38,52.52],[13.36,52.52],[13.36,52.50]]]}}'::text),
('green',
'{"type":"Feature","properties":{"name":"green","fillColor":"#3fa34d","strokeColor":"#14532d"},"geometry":{"type":"Polygon","coordinates":[[[13.39,52.50],[13.41,52.50],[13.41,52.52],[13.39,52.52],[13.39,52.50]]]}}'),
('blue',
'{"type":"Feature","properties":{"name":"blue","fillColor":"#2f6fdb","strokeColor":"#0b2f6b"},"geometry":{"type":"Polygon","coordinates":[[[13.42,52.50],[13.44,52.50],[13.44,52.52],[13.42,52.52],[13.42,52.50]]]}}'),
('magenta-rgba',
'{"type":"Feature","properties":{"name":"magenta-rgba","fillColor":[214,51,214,200],"strokeColor":[90,20,90,255]},"geometry":{"type":"Polygon","coordinates":[[[13.45,52.50],[13.47,52.50],[13.47,52.52],[13.45,52.52],[13.45,52.50]]]}}'),
('no-color',
'{"type":"Feature","properties":{"name":"no-color"},"geometry":{"type":"Polygon","coordinates":[[[13.36,52.47],[13.38,52.47],[13.38,52.49],[13.36,52.49],[13.36,52.47]]]}}'),
('point',
'{"type":"Feature","properties":{"name":"point","fillColor":"#111111"},"geometry":{"type":"Point","coordinates":[13.415,52.535]}}')
) AS t(name, geojson)
```
Automated:
```bash
cd superset-frontend
npm run test -- plugins/preset-chart-deckgl/src/layers/Geojson
```
Includes a regression test asserting the control's visibility resolves correctly without color_scheme_type)
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): restore the Fill Color control on the GeoJSON layer [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #42921:
URL: https://github.com/apache/superset/pull/42921#issuecomment-5228591069
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/42921?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 66.41%. Comparing base ([`a75665b`](https://app.codecov.io/gh/apache/superset/commit/a75665b014f142e3ae778b8d31c6f8c20860d278?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7682d54`](https://app.codecov.io/gh/apache/superset/commit/7682d548cd0fa8b72d5a6455f2eb427bd4bfb3d3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #42921 +/- ##
==========================================
- Coverage 66.42% 66.41% -0.01%
==========================================
Files 2858 2858
Lines 161342 161344 +2
Branches 37162 37162
==========================================
- Hits 107167 107163 -4
- Misses 52148 52154 +6
Partials 2027 2027
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/42921/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/42921/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.26% <100.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/42921?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): restore the Fill Color control on the GeoJSON layer [superset]
Posted by "DamianPendrak (via GitHub)" <gi...@apache.org>.
DamianPendrak commented on PR #42921:
URL: https://github.com/apache/superset/pull/42921#issuecomment-5263463076
Closing since fixed in https://github.com/apache/superset/pull/42712
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): restore the Fill Color control on the GeoJSON layer [superset]
Posted by "DamianPendrak (via GitHub)" <gi...@apache.org>.
DamianPendrak closed pull request #42921: fix(deckgl): restore the Fill Color control on the GeoJSON layer
URL: https://github.com/apache/superset/pull/42921
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): restore the Fill Color control on the GeoJSON layer [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42921:
URL: https://github.com/apache/superset/pull/42921#issuecomment-5228519761
<!-- Bito Reply -->
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`:
```typescript
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,
},
};
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): restore the Fill Color control on the GeoJSON layer [superset]
Posted by "floriandeutsch89 (via GitHub)" <gi...@apache.org>.
floriandeutsch89 commented on PR #42921:
URL: https://github.com/apache/superset/pull/42921#issuecomment-5237353554
> Thanks for working on this! It looks like a duplicate of #42712, which has already been reviewed - we're just waiting on its author to add unit tests. I'd suggest we keep that one and close this PR. That said, if your implementation covers something #42712 misses, or you think it's the better approach, let me know and I'll take another look
Oh you're right, too bad. Didn't find this one when looking at existing PRs.
Happy if this gets fixed somehow :-)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): restore the Fill Color control on the GeoJSON layer [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42921:
URL: https://github.com/apache/superset/pull/42921#discussion_r3741820935
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/controlPanel.ts:
##########
@@ -44,6 +44,18 @@ import {
import { dndGeojsonColumn } from '../../utilities/sharedDndControls';
import { BLACK_COLOR } from '../../utilities/controls';
+// The shared fillColorPicker is only shown when a `color_scheme_type` control is
+// set to "fixed color". The GeoJSON layer has no such control - features carry
+// their own colors and this picker acts as an override (set the opacity to 0 to
+// fall back to the feature's own fillColor) - so the gate is dropped here.
+const geojsonFillColorPicker = {
+ ...fillColorPicker,
+ config: {
+ ...fillColorPicker.config,
+ visibility: () => true,
+ },
+};
Review Comment:
**Suggestion:** The cloned control retains `fillColorPicker.config.default`, which is `PRIMARY_COLOR` with full opacity. When the control panel materializes this default for a GeoJSON chart, `getLayer` receives that value and replaces every feature's own `fillColor` with the teal override by default. The GeoJSON-specific control should default to a transparent color (or otherwise remain unset) so feature-provided colors are preserved until the user explicitly selects an override. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ GeoJSON features lose their configured fill colors by default.
- ⚠️ New GeoJSON charts render with the teal primary fill.
- ⚠️ The opacity-zero fallback is unavailable without manual adjustment.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8192cd9c12e646ba81d6149869f84dee&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8192cd9c12e646ba81d6149869f84dee&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/controlPanel.ts
**Line:** 51:57
**Comment:**
*Logic Error: The cloned control retains `fillColorPicker.config.default`, which is `PRIMARY_COLOR` with full opacity. When the control panel materializes this default for a GeoJSON chart, `getLayer` receives that value and replaces every feature's own `fillColor` with the teal override by default. The GeoJSON-specific control should default to a transparent color (or otherwise remain unset) so feature-provided colors are preserved until the user explicitly selects an override.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42921&comment_hash=a06f21aae68ee4814464eddccdfad728fcfe453dff08f61cfd4a92b8caff0181&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42921&comment_hash=a06f21aae68ee4814464eddccdfad728fcfe453dff08f61cfd4a92b8caff0181&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): restore the Fill Color control on the GeoJSON layer [superset]
Posted by "floriandeutsch89 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): restore the Fill Color control on the GeoJSON layer [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42921:
URL: https://github.com/apache/superset/pull/42921#issuecomment-5228502201
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org