Skip to content
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

feat(deckgl-map): use an arbitraty Mabpox style URL (#26027) #26031

Merged
merged 1 commit into from
Nov 28, 2023
Merged

feat(deckgl-map): use an arbitraty Mabpox style URL (#26027) #26031

merged 1 commit into from
Nov 28, 2023

Conversation

francois-travais
Copy link
Contributor

@francois-travais francois-travais commented Nov 20, 2023

SUMMARY

In the DeckGL map plugin we can only select a Mapbox style from a predefined list.

I've made the select box free form, meaning that we can copy-paste a Mapbox style URL directly. I've also added a basic validator for UX purpose.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Copy paste an arbitrary Mapbox URL in the select box and see the map change.

ADDITIONAL INFORMATION

  • Has associated issue: Fixes Use an arbitrary Mapbox styles URL in DeckGL maps #26027
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@john-bodley john-bodley requested a review from kgabryje November 20, 2023 17:53
@john-bodley
Copy link
Member

Thanks @francois-travais for the PR. I've cc'ed in somewhat who is likely well versed with DeckGL to review your change.

Copy link

codecov bot commented Nov 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (aad67e4) 69.10% compared to head (561731a) 69.10%.
Report is 4 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #26031   +/-   ##
=======================================
  Coverage   69.10%   69.10%           
=======================================
  Files        1940     1941    +1     
  Lines       75865    75873    +8     
  Branches     8442     8447    +5     
=======================================
+ Hits        52423    52431    +8     
+ Misses      21268    21267    -1     
- Partials     2174     2175    +1     
Flag Coverage Δ
javascript 56.28% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@francois-travais
Copy link
Contributor Author

Thanks @john-bodley . I know I'm missing a translation key (in the validator) but I cannot get babel script to work. I'm gonna try again today.

@francois-travais francois-travais marked this pull request as ready for review November 22, 2023 14:21
@francois-travais
Copy link
Contributor Author

@kgabryje Would you have time to look at this tiny PR please ? I would love to see this feature merged. Thanks

@kgabryje
Copy link
Member

@francois-travais Thank you for your contribution, looks really nice! I added 1 small suggestion, lmk if it makes sense

@kgabryje
Copy link
Member

1 more - wdyt about moving mapbox style controls to a separate line, so that the whole style URL fits in it? We can do it as a follow up if you prefer
MAP STYLE

Signed-off-by: François Travais <francois.travais@solinum.org>
@francois-travais
Copy link
Contributor Author

@kgabryje I modified the tooltip and moved Mapbox style select to a full line.

Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rusackas rusackas merged commit af58784 into apache:master Nov 28, 2023
@francois-travais francois-travais deleted the feat-arbitrary-mapbox-styles branch November 29, 2023 13:14
@francois-travais
Copy link
Contributor Author

@kgabryje do you know in which release this feature is planned ? I don't see it in 3.1.0rc3.

@kgabryje
Copy link
Member

kgabryje commented Jan 4, 2024

@kgabryje do you know in which release this feature is planned ? I don't see it in 3.1.0rc3.

CC @michael-s-molina

@michael-s-molina michael-s-molina added the v3.1 Label added by the release manager to track PRs to be included in the 3.1 branch label Jan 5, 2024
@michael-s-molina
Copy link
Member

@kgabryje do you know in which release this feature is planned ? I don't see it in 3.1.0rc3.

It will be available in the next RC.

michael-s-molina pushed a commit that referenced this pull request Jan 9, 2024
Signed-off-by: François Travais <francois.travais@solinum.org>
josedev-union pushed a commit to Ortege-xyz/studio that referenced this pull request Jan 22, 2024
…pache#26031)

Signed-off-by: François Travais <francois.travais@solinum.org>
fxprunayre added a commit to fxprunayre/superset that referenced this pull request Mar 12, 2024
In the DeckGL map plugin we can only select a Mapbox style from the [predefined list](https://github.com/apache/superset/blob/2499a1cf5a7f298c1ee2f34b3d67ca1d18bb7457/superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/Shared_DeckGL.jsx#L373-L380) or [typing a new Mapbox style URL](apache#26031).

This change add support for tile server layer making use of Deck.gl [TileLayer](https://deck.gl/docs/api-reference/geo-layers/tile-layer).
The well known OpenStreeMap layer provided by https://[abc].tile.openstreetmap.org/{z}/{x}/{y}.png is added to the list making easier for users not having a Mapbox account to have a background layer on map charts.

It has been tested with other tile server URL eg. tile://https://tile.osm.ch/name-it/{z}/{x}/{y}.png
fxprunayre added a commit to fxprunayre/superset that referenced this pull request Mar 14, 2024
In the DeckGL map plugin we can only select a Mapbox style from the [predefined list](https://github.com/apache/superset/blob/2499a1cf5a7f298c1ee2f34b3d67ca1d18bb7457/superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/Shared_DeckGL.jsx#L373-L380) or [typing a new Mapbox style URL](apache#26031).

This change add support for tile server layer making use of Deck.gl [TileLayer](https://deck.gl/docs/api-reference/geo-layers/tile-layer).
The well known OpenStreeMap layer provided by https://[abc].tile.openstreetmap.org/{z}/{x}/{y}.png is added to the list making easier for users not having a Mapbox account to have a background layer on map charts.

It has been tested with other tile server URL eg. tile://https://tile.osm.ch/name-it/{z}/{x}/{y}.png
fxprunayre added a commit to fxprunayre/superset that referenced this pull request Mar 14, 2024
In the DeckGL map plugin we can only select a Mapbox style from the [predefined list](https://github.com/apache/superset/blob/2499a1cf5a7f298c1ee2f34b3d67ca1d18bb7457/superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/Shared_DeckGL.jsx#L373-L380) or [typing a new Mapbox style URL](apache#26031).

This change add support for tile server layer making use of Deck.gl [TileLayer](https://deck.gl/docs/api-reference/geo-layers/tile-layer).
The well known OpenStreeMap layer provided by https://[abc].tile.openstreetmap.org/{z}/{x}/{y}.png is added to the list making easier for users not having a Mapbox account to have a background layer on map charts.

It has been tested with other tile server URL eg. tile://https://tile.osm.ch/name-it/{z}/{x}/{y}.png
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
…pache#26031)

Signed-off-by: François Travais <francois.travais@solinum.org>
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
…pache#26031)

Signed-off-by: François Travais <francois.travais@solinum.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L v3.1 Label added by the release manager to track PRs to be included in the 3.1 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants