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

Add new traces: scattermap, choroplethmap and densitymap which use maplibre to render maps #7060

Merged
merged 155 commits into from
Aug 8, 2024

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jul 22, 2024

Supersedes #7015.
Thanks to @birkskyum's PR, I kept mapbox traces and added new traces i.e. scattermap, choroplethmap and densitymap to the API.
These traces use map, map2, etc. on the layout and do not require a config parameter as they use maplibre-gl.

@plotly/plotly_js

Comment on lines +35 to +40
'Defines the map layers that are rendered by default below the trace layers defined in `data`,',
'which are themselves by default rendered below the layers defined in `layout.map.layers`.',
'',
'These layers can be defined either explicitly as a Map Style object which can contain multiple',
'layer definitions that load data from any public or private Tile Map Service (TMS or XYZ) or Web Map Service (WMS)',
'or implicitly by using one of the built-in style objects which use WMSes',
Copy link
Member

Choose a reason for hiding this comment

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

@birkskyum I was wondering about this: "map layers that are rendered by default below the trace layers defined in data"

When testing the following example out on the Python side:


import pandas as pd
import plotly.graph_objects as go

quakes = pd.read_csv('https://raw.githubusercontent.com/plotly/datasets/master/earthquakes-23k.csv')


fig = go.Figure(go.Densitymap(lat=quakes.Latitude, lon=quakes.Longitude, z=quakes.Magnitude, radius=10))

fig.update_layout(
    map={
        "style": "carto-darkmatter",
        "center": {"lon": 180},
    },
    margin={"r": 0, "t": 0, "l": 0, "b": 0}
)

fig.show()

I see this, where it looks like the text is above the data. Is that expected.

image

The previous implementation looked like this

image

Copy link
Contributor

@birkskyum birkskyum Jul 30, 2024

Choose a reason for hiding this comment

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

When adding a layer to mapbox/maplibre, it's possible to specify which layer in the style it should sit below - this is thus dependent on which style is used. In this case, with the carto dark matter style, that setting should likely be 'waterway_label' to give the expected outcome. The primary tool used to examine the individual layers of a style.json, and make new styles, is Maputnik.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally like this new look; but we should possibly try to make the maplibre version look similar to the mapbox one. @birkskyum Do you know what may change in the mock or code to make that happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@archmoj
Copy link
Contributor Author

archmoj commented Jul 30, 2024

@ndrezn @emilykl attributions added in #4069 (See https://github.com/plotly/plotly.js/pull/4069/files#diff-084161b3ee075801aa5bf2cc0ea0b232ea5a770a76a98785a09b132f858c36f2) was required in respect to mapbox.
But using maplibre in new traces I think the desirable default behavior should be not have any attribution over the map subplot. What's your opinion on it?

@archmoj
Copy link
Contributor Author

archmoj commented Aug 7, 2024

@emilykl @ndrezn The tests are green on this PR.
Please note I would add deprecation notes on a separate PR.

@archmoj archmoj requested a review from emilykl August 7, 2024 13:10
@ndrezn
Copy link
Member

ndrezn commented Aug 8, 2024

But using maplibre in new traces I think the desirable default behavior should be not have any attribution over the map subplot. What's your opinion on it?

Not sure if this was resolved but I agree that the desirable default is no attribution. @birkskyum do you have any thoughts on this?

Copy link
Contributor

@emilykl emilykl left a comment

Choose a reason for hiding this comment

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

Looks great. 👍 🌟 💫

@birkskyum
Copy link
Contributor

birkskyum commented Aug 8, 2024

Not sure if this was resolved but I agree that the desirable default is no attribution. @birkskyum do you have any thoughts on this?

The icon in the lower left corner is entirely optional, so if it's preferable to the product to leave it out, then that seems like the right thing to do. Like with other BSD/MIT/Apache licensed software, the only ask is really to retain/reproduce the copyright notice somewhere, but apart from that the only concern is what's best for the end-user. In the case of plotly.js, where each graph typically is rather small, and there are often multiple graphs, then the icon takes up more space / makes more noise than i.e. a full screen app like https://maps.bing.com (they also opted not to show the icon, but it would have taken up relatively less space). The style/data attribution text/toggle (lower right) is dependent on the style provider, so it's probably a good idea to keep that around.

@archmoj
Copy link
Contributor Author

archmoj commented Aug 8, 2024

@birkskyum @alexcjohnson
FYI - This PR is approved by @emilykl and other team members to release a RC.
Remaining improvements e.g. hiding attributions by default, removing constant stamen styles keys that require API key, deprecation notices should be addressed in upcoming PRs after the release of this RC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants