Greater-London-Authority / ldn-viz-tools

https://greater-london-authority.github.io/ldn-viz-tools/
1 stars 0 forks source link

Add MapPopup and associated components #358

Closed PaulioRandall closed 3 months ago

PaulioRandall commented 3 months ago

What does this change?

Adds the <MapPopup> and supporting components. This component uses the MapCursor functionality to create tooltips and markers on the map as users hover and click features. It also adds:

Why?

Because creating tooltips and markers is verbose and painful. It becomes much more difficult for apps with many overlapping layers and tooltips.

How is it tested?

Storybook + live applications

How is it documented?

Storybook

Are light and dark themes considered?

No.

Is it complete?

jamesscottbrown commented 3 months ago

I'm finding the naming here confusing.

As I understand it:

This doesn't quite match the naming in the current implementation, which has a MapPopup component with tooltip and marker props.

I think it would be better to:

PaulioRandall commented 3 months ago

I'm finding the naming here confusing.

As I understand it:

  • A marker is something drawn on the map to represent something at a particular location.
  • A tooltip displays contextual information whilst a user hovers over a marker; it closes as soon as the cursor moves away from the marker.
  • A popup displays contextual information when a user clicks on a marker; it remains open until dismissed (by clicking the close icon, pressing the Escape key, or clicking elsewhere on the page.

This doesn't quite match the naming in the current implementation, which has a MapPopup component with tooltip and marker props.

I think it would be better to:

  • rename the MapPopup component to Marker (it isn't just a popup, and might have a tooltip but no popup)
  • keep the tooltip prop, as the component used to render a tooltip on hover
  • rename the marker prop to popup, as this is the component used to render the popop on click

@jamesscottbrown I've applied this change. I've also renamed context labels to be more precise about what MapLibre type they contain.

jamesscottbrown commented 3 months ago

Currently, there is:

I see why there are two objects: there may be bother a tooltip and popup with different contents; and a mousout/leaveTopFeature needs to close a tooltip (if open), but not a popup.

However, I don't see why they aren't both maplibre_gl.Popup objects. Changing renderPopup to match renderTooltip (by creating a maplibre_gl.Popup object and setting container to be a newly created div) seems to work fine.

I'd suggest having 2 maplibre_gl.Popup objects, called tooltipMaplibrePopup and popupMaplibrePopup, and passed into the context with names that indicate whether they correspond to a tooltip or popup.

PaulioRandall commented 3 months ago

Currently, there is:

  • a maplibre_gl.Popup object called tooltipMaplibrePopup (passed into a context as mapMarkerMaplibrePopup)
  • a maplibre_gl.Marker object called popupMaplibreMarker (passed into a context as mapMarkerMaplibreMarker)

I see why there are two objects: there may be bother a tooltip and popup with different contents; and a mousout/leaveTopFeature needs to close a tooltip (if open), but not a popup.

However, I don't see why they aren't both maplibre_gl.Popup objects. Changing renderPopup to match renderTooltip (by creating a maplibre_gl.Popup object and setting container to be a newly created div) seems to work fine.

I'd suggest having 2 maplibre_gl.Popup objects, called tooltipMaplibrePopup and popupMaplibrePopup, and passed into the context with names that indicate whether they correspond to a tooltip or popup.

Currently, there is:

  • a maplibre_gl.Popup object called tooltipMaplibrePopup (passed into a context as mapMarkerMaplibrePopup)
  • a maplibre_gl.Marker object called popupMaplibreMarker (passed into a context as mapMarkerMaplibreMarker)

I see why there are two objects: there may be bother a tooltip and popup with different contents; and a mousout/leaveTopFeature needs to close a tooltip (if open), but not a popup.

However, I don't see why they aren't both maplibre_gl.Popup objects. Changing renderPopup to match renderTooltip (by creating a maplibre_gl.Popup object and setting container to be a newly created div) seems to work fine.

I'd suggest having 2 maplibre_gl.Popup objects, called tooltipMaplibrePopup and popupMaplibrePopup, and passed into the context with names that indicate whether they correspond to a tooltip or popup.

@jamesscottbrown I've updated the code with this change.