-
Notifications
You must be signed in to change notification settings - Fork 16.6k
fix(charts): numerical column for the Point Radius field in mapbox #36962
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix(charts): numerical column for the Point Radius field in mapbox #36962
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.jsx
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.jsx
Outdated
Show resolved
Hide resolved
|
CodeAnt AI finished reviewing your PR. |
Code Review Agent Run #6faee1Actionable 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 |
|
Would appreciate it if you can check the bot review comments and either reply or address them, and get pre-commit to pass. The giant circles are a little overwhelmingly large in some cases, but if they're accurate, they're accurate. I wonder if there's a way to set opacity or something to make them more useful in this regard? Then you could see where you are on the map when they cover the whole chart, at least. Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following: A series of checks will now run when you make a git commit. Alternatively it is possible to run pre-commit by running pre-commit manually: |
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Now that you mentioned opacity, I found an issue in the “Visual Tweaks” section. The opacity field is present, but regardless of the value you set, the visual opacity remains the same. I’ll open a separate PR to address this issue. |
Code Review Agent Run #4cfd2dActionable 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 |
| const maxLabel = Math.max(...clusterLabelMap.filter(v => !Number.isNaN(v))); | ||
|
|
||
| // Calculate min/max radius values for Pixels mode scaling | ||
| let minRadiusValue = Infinity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some tests?
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.jsx
Outdated
Show resolved
Hide resolved
Code Review Agent Run #c147baActionable 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 |
Code Review Agent Run #5c6437Actionable 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 |
|
🎪 Showtime deployed environment on GHA for eb7f1af • Environment: http://34.219.31.62:8080 (admin/admin) |
EnxDev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FelipeGLopez should we add a test to ensure that points remain visible when the radius is set to extreme values?
PS: The test should validate the user-facing behavior, verifying that points are still rendered and accessible from the user’s perspective, rather than asserting on implementation details.
Yes, you have a point there! I added that missing test and changed the tests. |
Code Review Agent Run #fd214eActionable Suggestions - 0Additional Suggestions - 1
Review 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 |
SUMMARY
The code passed raw numerical column values as the base pointRadius for all units (
Kilometers,Miles, andPixels), causing points to render at extreme sizes or disappear entirely.The fix has two parts:
(1) In
transformProps.js, it always usesDEFAULT_POINT_RADIUSas the base radius regardless of unit, then lets individual points get their values from geoJSON properties(2) In
ScatterPlotGlowOverlay.jsx, it adds normalization specifically for Pixels mode (min/max scaling to 5px-radius/3 range), whileKilometersandMileswere already being converted to pixels viacomputePointRadiusPixels()but now work correctly because they're based on the normalizedDEFAULT_POINT_RADIUSinstead of raw data values.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
Currently, Mapbox does not display anything when a numerical column other than
Autois selected, regardless of the Point Radius Unit.Before.mov
After:
Here, we select a numerical column (
radius_miles), and the points are correctly rendered.After.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION