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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions lonboard/_map.py
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,44 @@ def __init__(
[`lonboard.basemap.CartoBasemap.PositronNoLabels`][lonboard.basemap.CartoBasemap.PositronNoLabels]
"""

custom_attribution = traitlets.Union(
[
traitlets.Unicode(allow_none=True),
traitlets.List(traitlets.Unicode(allow_none=False)),
]
).tag(sync=True)
Comment on lines +170 to +175
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?

"""
Custom attribution to display on the map.

This attribute supports the same format as the `attribution` property in the
Maplibre API.

- Type: `str` or `List[str]`
- Default: `None`

You can provide either a single string or a list of strings for custom attributions.
If an attribution value is set in the map style, it will be displayed in addition to
this custom attribution.

**Example:**

```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.

)
```

**Example:**

```py
m = Map(
layers,
custom_attribution=["Development Seed", "OpenStreetMap"]
)
```
"""

# TODO: We'd prefer a "Strict union of bool and float" but that doesn't
# work here because `Union[bool, float]` would coerce `1` to `True`, which we don't
# want, and `Union[float, bool]` would coerce `True` to `1`, which we also don't
Expand Down
7 changes: 6 additions & 1 deletion src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { v4 as uuidv4 } from "uuid";
import { Message } from "./types.js";
import { flyTo } from "./actions/fly-to.js";
import { useViewStateDebounced } from "./state";
import "maplibre-gl/dist/maplibre-gl.css";

await initParquetWasm();

Expand Down Expand Up @@ -71,6 +72,7 @@ function App() {
let [pickingRadius] = useModelState<number>("picking_radius");
let [useDevicePixels] = useModelState<number | boolean>("use_device_pixels");
let [parameters] = useModelState<object>("parameters");
let [customAttribution] = useModelState<string>("custom_attribution");

// initialViewState is the value of view_state on the Python side. This is
// called `initial` here because it gets passed in to deck's
Expand Down Expand Up @@ -184,7 +186,10 @@ function App() {
}}
parameters={parameters || {}}
>
<Map mapStyle={mapStyle || DEFAULT_MAP_STYLE} />
<Map
mapStyle={mapStyle || DEFAULT_MAP_STYLE}
customAttribution={customAttribution}
></Map>
</DeckGL>
</div>
);
Expand Down
2 changes: 0 additions & 2 deletions src/model/base-layer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,6 @@ export abstract class BaseLayerModel extends BaseModel {
}

baseLayerProps(): LayerProps {
// console.log("extensions", this.extensionInstances());
// console.log("extensionprops", this.extensionProps());
return {
extensions: this.extensionInstances(),
...this.extensionProps(),
Expand Down