fix(deckgl): remove visibility condition in deckgl fill color - #42712
fix(deckgl): remove visibility condition in deckgl fill color#42712MallikarjunaReddyN wants to merge 2 commits into
Conversation
dosubot
Bot
added
change:frontend
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #ffd628
Actionable Suggestions - 1
-
superset-frontend/plugins/preset-chart-deckgl/src/utilities/Shared_DeckGL.tsx - 1
- Control visibility logic removed · Line 283-287
Review Details
-
Files reviewed - 1 · Commit Range:
8fa4297..8fa4297- superset-frontend/plugins/preset-chart-deckgl/src/utilities/Shared_DeckGL.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42712 +/- ##
==========================================
- Coverage 65.43% 65.43% -0.01%
==========================================
Files 2810 2810
Lines 159528 159529 +1
Branches 36410 36412 +2
==========================================
Hits 104380 104380
- Misses 53102 53103 +1
Partials 2046 2046
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
DamianPendrak
left a comment
DamianPendrak
left a comment
There was a problem hiding this comment.
That is a real issue, but removing visibility check makes it show in Polygon chart even when linear color scheme is selected.
I propose checking whether the control exists, it enables the visibility when there is no color scheme control:
visibility: ({ controls }) => !controls.color_scheme_type || isColorSchemeTypeVisible(controls, COLOR_SCHEME_TYPES.fixed_color),
…th Polygon and GeoJSON charts
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@DamianPendrak Good catch, thanks! I've updated the implementation to check for the existence of the color_scheme_type control: This keeps the existing behavior for Polygon charts while correctly enabling the control for GeoJSON charts, where color_scheme_type is not present. |
Code Review Agent Run #5dbe06Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| default: PRIMARY_COLOR, | ||
| renderTrigger: true, | ||
| visibility: ({ controls }) => | ||
| !controls.color_scheme_type || |
There was a problem hiding this comment.
This fixes a user-visible regression through an untested two-branch predicate, so the GeoJSON picker can disappear again or the Polygon picker can show for non-fixed schemes without a relevant Jest failure. Could we add a focused fillColorPicker.config.visibility test covering an absent color_scheme_type, fixed_color, and the non-fixed values?
3 participants
SUMMARY
This PR fixes an issue where the Fill Color picker was not rendered in the Deck.gl GeoJSON chart controls.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION