fix(deckgl): polygon elevation fixed value #35266
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Suboptimal Union Type Design ▹ view | 🧠 Not in scope |
 |
Missing legacy metric handling in elevation logic ▹ view | 🧠 Not in scope |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Polygon/buildQuery.ts | ✅ |
| superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Polygon/transformProps.ts | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| export interface DeckPolygonFormData extends SqlaFormData { | ||
| line_column?: string; | ||
| line_type?: string; | ||
| metric?: string; | ||
| point_radius_fixed?: { | ||
| value?: string; | ||
| }; | ||
| point_radius_fixed?: | ||
| | { | ||
| value?: string; | ||
| } | ||
| | { | ||
| type: 'fix'; | ||
| value: string; | ||
| } | ||
| | { | ||
| type: 'metric'; | ||
| value: QueryFormMetric; | ||
| }; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if ('type' in point_radius_fixed) { | ||
| if ( | ||
| point_radius_fixed.type === 'metric' && | ||
| point_radius_fixed.value != null | ||
| ) { | ||
| metrics.push(point_radius_fixed.value); | ||
| } | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
michael-s-molina
requested review from
Copilot,
kgabryje and
michael-s-molina
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue with deck.gl polygon chart elevation when set to a fixed value. The problem was that fixed elevation values (like "1000") were not being properly handled, resulting in no elevation being applied to polygons.
- Adds proper support for fixed elevation values in the polygon transform props logic
- Updates type definitions to handle both fixed values and metric-based elevation configurations
- Implements comprehensive test coverage for elevation handling scenarios
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
transformProps.ts |
Adds fixed elevation value parsing and conditional logic to prioritize fixed values over metric-based elevation |
transformProps.test.ts |
New comprehensive test suite covering fixed elevation, metric elevation, and edge cases |
buildQuery.ts |
Updates type definitions to support both legacy and new elevation configuration structures |
buildQuery.test.ts |
New comprehensive test suite for query building with various elevation configurations |
| point_radius_fixed?: | ||
| | { | ||
| value?: string; | ||
| } | ||
| | { | ||
| type: 'fix'; | ||
| value: string; | ||
| } | ||
| | { | ||
| type: 'metric'; | ||
| value: QueryFormMetric; | ||
| }; |
There was a problem hiding this comment.
The union type has inconsistent nullability for the value field. The first variant allows value?: string (optional), while the second variant requires value: string. This inconsistency could lead to runtime errors. Consider making the value field consistently optional or required across all variants.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
The optional value type makes it backward compatible and it's handled properly in Polygon's transformProps and FixedOrMetricControl
1 participant
SUMMARY
I discovered an issue with the deck.gl polygon chart's elevation when set to a fixed value like "1000". This PR resolves that.
BEFORE/AFTER
Before
After
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION