-
Notifications
You must be signed in to change notification settings - Fork 16.4k
feat(deckgl): make map tiles configurable #32867
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
Conversation
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.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Eager DOM Access and JSON Parse ▹ view | ✅ Fix detected | |
| Hardcoded map style string literal with unclear comparison ▹ view | 🧠 Not in scope | |
| Ineffective Fallback MapStyle ▹ view | 🧠 Not in scope | |
| Direct Props Mutation in Layers Function ▹ view | 🧠 Incorrect | |
| Counter-intuitive validation return values ▹ view | 🧠 Not in standard |
Suppressed issues based on your team's Korbit activity
| This issue | Is similar to | Because |
|---|---|---|
|
The code assumes data-bootstrap attribute contains valid JSON, but doesn't handle JSON.parse exceptions which could crash the application. |
Similar issues were not addressed in the past |
When you react to issues (for example, an upvote or downvote) or you fix them, Korbit will tune future reviews based on these signals.
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/packages/superset-ui-core/src/validator/validateMapboxStylesUrl.ts | ✅ |
| superset-frontend/plugins/legacy-preset-chart-deckgl/src/DeckGLContainer.tsx | ✅ |
| superset-frontend/plugins/legacy-preset-chart-deckgl/src/utils.ts | ✅ |
| superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/Shared_DeckGL.jsx | ✅ |
| superset/views/base.py | ✅ |
| superset/config.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Feedback and Support
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/Shared_DeckGL.jsx
Outdated
Show resolved
Hide resolved
superset-frontend/packages/superset-ui-core/src/validator/validateMapboxStylesUrl.ts
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@mistercrunch Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
|
Did a quick pass reviewing the PR, and overall seems like a great feature! Curious, are these tiles served for free, no API key required? If it's all free and open, it seems like a better default over mapbox maybe (?) About the NOTE: I fired up an ephemeral environment so anyone can take the feature for a test drive. Though I'm realizing that mapbox doesn't seem to work out-of-the-box, curious whether we can make OSM work in eph-envs & dev-envs. |
|
Errors in CI point to having to |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #32867 +/- ##
===========================================
+ Coverage 0 83.18% +83.18%
===========================================
Files 0 553 +553
Lines 0 39999 +39999
===========================================
+ Hits 0 33272 +33272
- Misses 0 6727 +6727
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This helps catching these errors as you commit -> https://superset.apache.org/docs/contributing/development/#git-hooks |
|
@plavacquery Thanks for the contribution. Could you add the summary of the PR (and might be adding the screenshot of the changes)? |
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/Shared_DeckGL.jsx
Outdated
Show resolved
Hide resolved
|
I understand correctly that after this revision, it will be possible to add styles from mapbox "mapbox://styles/..." to the superset directly via config_superset.ru? If it has already been implemented. can you tell me how I can use the required style? Or is there another way to change the map language? |
|
What is |
9039dab to
4e6167f
Compare
830a99f to
2244d2c
Compare
|
@mistercrunch Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
|
Triggering CI and generating an ephemeral environment to test the feature. Wondering if we should make this the default given that deckgl+mapbox doesn't work out of the box. Would be a much better experience for people trying out Superset to have an actual map out of the box without having to get/configure a mapbox api key. Might make sense to set this new default but make it configurable for those who have an API key and don't want openstreetmap as the default. Would require a new default and a note in |
|
@mistercrunch Ephemeral environment spinning up at http://52.32.20.64:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
|
mmmh, ephemeral env has an issue from when the |
|
@mistercrunch So we can open a SIP that will make default osm tiles for deckgl and allow switching to mapbox tiles when a mapbox token is set in config.py file. And in the meantime can we merge this MR that allow user to use OSM or other internal tiles with config.py ? The CI has passed |
|
I don't think a SIP is required. I think a note in So moving forward and in-scope for this PR:
I think we can get consensus on this PR and move forward with all this (no need for a SIP) |
20bce50 to
454177c
Compare
|
@mistercrunch Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
|
Saw new commits so I approved CI run and fired up a fresh ephemeral environment |
|
@mistercrunch Ephemeral environment spinning up at http://34.219.197.19:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
|
Hello ! This feature looks great ! Thanks @plavacquery ! |
|
The 3 points from my earlier comments - if we all agree -> point 3 requires a new comment in Once that's all set up, I should be able to create an ephemeral environment or fire up |
8d244e5 to
edf4107
Compare
4f7fc01 to
f30588e
Compare
f8a7a29 to
0fa3feb
Compare
SUMMARY
Allow to use different tiles than mapbox with deckgl map. Tiles informations can be add in superset config.py
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
In config.py set the following
And in TALISMAN_DEV_CONF
add "https://c.tile.openstreetmap.org" to 'connect-src' array
ADDITIONAL INFORMATION