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

Fix attribution #561

Merged
merged 5 commits into from
Jul 4, 2024
Merged

Fix attribution #561

merged 5 commits into from
Jul 4, 2024

Conversation

vgeorge
Copy link
Member

@vgeorge vgeorge commented Jul 1, 2024

This adds a custom_attribution property to the Map class and fix Maplibre CSS by adding an import. Contributes to:

To test, add the custom_attribution property to a Map instance as demonstrated in the API docs examples.

@kylebarron this is ready for review.

@vgeorge vgeorge requested a review from kylebarron July 1, 2024 11:42
Comment on lines +170 to +175
custom_attribution = traitlets.Union(
[
traitlets.Unicode(allow_none=True),
traitlets.List(traitlets.Unicode(allow_none=False)),
]
).tag(sync=True)
Copy link
Member

Choose a reason for hiding this comment

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

Does the attribution need to support hyperlinking too? I.e. do we need to define not just text but also the target URL?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kylebarron the MapLibre attribution control renders html links if the attribution text contains an element. They are not appearing now because the Map container has z-index: -1, making the DeckGL canvas to be placed in front of the map controllers, blocking mouse events on it. In the recording below I disabled the z-index, but that caused the data layer to disappear. The z-index is not defined in Lonboard code, I would suggest we address this issue in separate because resolving the z-index conflict might require a more complex solution that ensures both the DeckGL canvas and MapLibre controllers are correctly rendered and interactive.

hyperlink

Copy link
Member

Choose a reason for hiding this comment

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

Right. I think this is again tied to #437. The "proper" way as of the latest deck.gl versions is to use MapboxOverlay, which has the pros and cons described in #437. I think when you use MapboxOverlay then the maplibre attribution control is clickable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Apologies, I haven't looked into #437 in depth. I believe we can implement a custom attribution control, which would untie us from MapLibre. We would need a way to extract attribution from map styles JSON, but I think it is doable. Should we keep discussing this on #561?

@kylebarron
Copy link
Member

This is also quite tied to the decision here: #437, of whether we should fully tie ourselves to Maplibre or not, or whether we should at least provide an option for ourselves in the future to have deck-only rendering without Maplibre

@vgeorge
Copy link
Member Author

vgeorge commented Jul 3, 2024

@kylebarron If you are concerned about having the parameter custom_attribution as part of the Map class API, I would say it doesn't tie us much to MapLibre, because all other major open-source mapping libraries (OpenLayers, Leaflet) accept HTML text in attribution controls. Additionally, if we need to use Deck.gl directly, it should be straightforward to implement an Attribution control that accepts HTML text.

```py
m = Map(
layers,
custom_attribution="Development Seed"
Copy link
Member

Choose a reason for hiding this comment

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

Should we maybe mention that this custom_attribution string can be HTML? Or should we wait on that until the link is actually clickable with fixed z-index?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe we should mention it once it is supported properly. Let's keep tracking it on #561.

@vgeorge vgeorge merged commit 98ba471 into main Jul 4, 2024
5 checks passed
@vgeorge vgeorge deleted the fix/map-attribution branch July 4, 2024 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants