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

Updates for maplibre maps #4706

Merged
merged 45 commits into from
Aug 29, 2024
Merged

Updates for maplibre maps #4706

merged 45 commits into from
Aug 29, 2024

Conversation

LiamConnors
Copy link
Member

@LiamConnors LiamConnors commented Aug 5, 2024

  • Update docs
  • Add new px functions

@LiamConnors LiamConnors requested a review from archmoj August 5, 2024 20:19
@LiamConnors LiamConnors changed the title Maplibre tests Maplibre updates Aug 5, 2024
@LiamConnors LiamConnors changed the title Maplibre updates Docs updates for new maps Aug 5, 2024
@LiamConnors LiamConnors requested review from emilykl and ndrezn August 5, 2024 20:19
doc/python/scattermapbox.md Outdated Show resolved Hide resolved
@archmoj
Copy link
Contributor

archmoj commented Aug 6, 2024

Great PR. Thanks @LiamConnors for all this. 🏅 🏆 🌟
I suggest we set status of this PR to on hold as we did for plotly/plotly.js#7015 and don't merge it.
That way we could simply look at the main differences between the two.

Then you could duplicate your branch, rename all the docs files that has mapbox in their name to map and then bring back the mapbox docs from master and open a new pull request.
This way we have docs for both mapbox and new maps on the other PR.
Thank you!

@archmoj
Copy link
Contributor

archmoj commented Aug 6, 2024

Here is a list of doc files that has mapbox in their name:

./doc/python/mapbox-county-choropleth.md
./doc/python/mapbox-layers.md
./doc/python/filled-area-on-mapbox.md
./doc/python/mapbox-density-heatmaps.md
./doc/python/lines-on-mapbox.md
./doc/python/scattermapbox.md
./doc/python/hexbin-mapbox.md

@LiamConnors
Copy link
Member Author

Here is a list of doc files that has mapbox in their name:

./doc/python/mapbox-county-choropleth.md
./doc/python/mapbox-layers.md
./doc/python/filled-area-on-mapbox.md
./doc/python/mapbox-density-heatmaps.md
./doc/python/lines-on-mapbox.md
./doc/python/scattermapbox.md
./doc/python/hexbin-mapbox.md

I think leaving the file names as is is okay for now as they are not displayed in the docs and it makes it easier for anyone reviewing this PR to see what's changed

@archmoj
Copy link
Contributor

archmoj commented Aug 6, 2024

Yes we could keep mapbox names in this PR. But for the other one we want to merge those should be renamed as we need to keep doc for mapbox traces.

@LiamConnors
Copy link
Member Author

Yes we could keep mapbox names in this PR. But for the other one we want to merge those should be renamed as we need to keep doc for mapbox traces.

This PR keeps some mapbox examples. Like here https://github.com/plotly/plotly.py/pull/4706/files#diff-aeb7417e2580482c1fb42c4b976c4e0beb113fcb7333efee59c57bc06d3e04e6R138-R161
But I'm not sure we need to keep all examples written using Mapbox traces and the main focus of the pages should now be the new traces.
What do you think @ndrezn @emilykl

@archmoj
Copy link
Contributor

archmoj commented Aug 6, 2024

IMHO, for a while we need to keep mapbox examples until they could use the next plotly.py version in their systems.
@ndrezn @emilykl @gvwilson

@archmoj
Copy link
Contributor

archmoj commented Aug 6, 2024

Also having two versions (mapbox and new map) on two doc files, could help everyone see the difference between the two.

@ndrezn
Copy link
Member

ndrezn commented Aug 6, 2024

We should keep some mapbox examples, but like in plotly.py I would expect a deprecation notice associated with them. With the major version where we fully drop support for Mapbox I would also expect we drop these examples (and note in the new docs that these maps replace the previous Mapbox trace types with feature parity).

@archmoj
Copy link
Contributor

archmoj commented Aug 6, 2024

There is also hexbin_mapbox in figure_factory at packages/python/plotly/plotly/figure_factory/_hexbin_mapbox.py.
We should possibly add hexbin_map. No?

@gvwilson
Copy link
Contributor

gvwilson commented Aug 6, 2024

I'm OK with shipping most of the maps in this release and doing another with the small number of maps we didn't get to.

doc/python/axes.md Outdated Show resolved Hide resolved
Copy link
Contributor

@archmoj archmoj left a comment

Choose a reason for hiding this comment

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

I have one remaining comment here: #4706 (comment)

Other than that it looks great and works great too!

You may consider merging #4726 into this branch or target master after merging this PR.

@LiamConnors LiamConnors requested review from ndrezn and emilykl August 28, 2024 17:19
doc/python/axes.md Outdated Show resolved Hide resolved
LiamConnors and others added 4 commits August 29, 2024 09:31
Add new `map` options to `px` and update plotly.js/master at 056799dfc705a4533935ae3169cc94d35bc44830
@LiamConnors LiamConnors changed the title Docs updates for new maps Updates for maplibre maps Aug 29, 2024
@LiamConnors LiamConnors merged commit a747fea into master Aug 29, 2024
4 of 5 checks passed
@LiamConnors LiamConnors deleted the maplibre-tests branch August 29, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation written for humans feature something new P1 needed for current cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants