dimfeld / svelte-maplibre

Svelte bindings for the MapLibre mapping library
https://svelte-maplibre.vercel.app
MIT License
352 stars 40 forks source link

Allow developer to define how the map should react to updated props #149

Open singingwolfboy opened 7 months ago

singingwolfboy commented 7 months ago

Right now, the MapLibre.svelte component has this code in it:

$: if (center && !compare(center, $mapInstance?.getCenter())) $mapInstance?.panTo(center);
$: if (zoom && !compare(zoom, $mapInstance?.getZoom())) $mapInstance?.zoomTo(zoom);
$: if (bounds && !compare(bounds, $mapInstance?.getBounds())) $mapInstance?.fitBounds(bounds);

Because of this code, if the center, zoom, or bounds props are modified, the map automatically calls panTo(), zoomTo(), or fitBounds() with the new props. This is a very good default, and is probably the right behavior 80% of the time -- but sometimes, a developer may want (or need) to modify this behavior. For example, they may need to set a duration for the animation, or call jumpTo() instead of panTo(), or even prevent the map from moving in certain cases.

There should be a way for a developer to define how the map reacts to these updated props, in cases where the default behaviors are not sufficient. I'm honestly unsure of the best way to design this API, but I thought I'd raise an issue so we could discuss it!

dimfeld commented 7 months ago

Yeah I agree this is a nice feature to have. Here are two possibilities; I'm not sure yet which one I prefer (or something else I haven't thought of!).

Option to Disable Reactivity

interface Reactive {
  center?: boolean;
  zoom?: boolean;
  bounds?: boolean;
}

export let reactive: boolean | Reactive = true;

$: isReactive = typeof reactive === 'boolean' 
  ? { center: reactive, zoom: reactive, bounds: reactive }
  : reactive.

$: if (center && isReactive.center && !compare(center, $mapInstance?.getCenter())) $mapInstance?.panTo(center);
$: if (zoom && isReactive.zoom && !compare(zoom, $mapInstance?.getZoom())) $mapInstance?.zoomTo(zoom);
$: if (bounds && isReactive.bounds && !compare(bounds, $mapInstance?.getBounds())) $mapInstance?.fitBounds(bounds);

Pros and Cons:

Custom Callback

Another possibility would be the option to supply a custom callback to apply these changes, which would be called when any of them change. Something like this:

export let applyBounds: (({ map, zoom, center, bounds }) => unknown) | undefined = undefined;

$: if (applyBounds && (
   (center && !compare(center, $mapInstance?.getCenter()) ||
   (zoom && !compare(zoom, $mapInstance?.getZoom()) ||
   (bounds && !compare(bounds, $mapInstance?.getBounds()) ) {
  applyBounds({ map: $mapInstance, center, zoom, bounds });
}

$: if (!applyBounds && center && !compare(center, $mapInstance?.getCenter())) $mapInstance?.panTo(center);
$: if (!applyBounds && zoom && !compare(zoom, $mapInstance?.getZoom())) $mapInstance?.zoomTo(zoom);
$: if (!applyBounds && bounds && !compare(bounds, $mapInstance?.getBounds())) $mapInstance?.fitBounds(bounds);

Pros and Cons of this approach

Given that last con, I think the first option to just allow disabling reactivity could work the best.

dimfeld commented 7 months ago

Planning to implement this next week

singingwolfboy commented 7 months ago

I found a way to work around this, by not passing these props and using the on:load callback instead. Specifically, I did this:

<MapLibre
  style={{...}}
  on:load={(event) => {
    const map = event.detail;
    feature.subscribe(($feature) => {
      if ($feature) {
        const bounds = $feature.properties.bounds;
        if (bounds) {
          const camera = map.cameraForBounds(bounds);
          map.jumpTo(camera);
        } else {
          map.jumpTo({
            center: $feature.geometry.coordinates
          });
        }
      }
    });
  }}
/>

Since there is a workaround to allow the developer to take full control of how to handle these props changing, it's perfectly reasonable to expose a simpler API that is limited in its flexibility. Maybe this would be enough:

<MapLibre
  style={{...}}
  center={$center}
  bounds={$bounds}
  animationOptions={{
    center: {
      duration: 500,
    },
    bounds: {
      animate: false
    }
  }}
/>

Where this animationOptions object has keys of center, bounds, and zoom (and possibly others?), and the values are AnimationOptions objects. Naturally, these values are passed to the panTo(), zoomTo(), and fitBounds() functions.

For developers who need more control than this, it's probably enough to clearly document how they can use the on:load callback to do it manually, the way I did.

dimfeld commented 7 months ago

Glad you found something that worked, and thanks for the suggestion on the API! That does feel like a good middle ground while leaving the option to do something more advanced like what you ended up doing.

larsmaxfield commented 6 months ago

This automatic behavior also seems to be responsible for unwanted movement when viewing 3D terrain:

svelte-maplibre-auto-movement-short

Adding the suggested option to disable reactivity (and then disabling it) stops this from happening.

timadevelop commented 6 months ago

Maybe I'm wrong but it seems like the current behaviour

$: if (center && !compare(center, $mapInstance?.getCenter())) $mapInstance?.panTo(center);
$: if (zoom && !compare(zoom, $mapInstance?.getZoom())) $mapInstance?.zoomTo(zoom);

does not allow changing both zoom and map center together. Maplibre will zoom properly, fire a moveend event, and forget about finishing panning to the center.

e.g. clicking the button below will zoom but panning won't be performed properly

//...
  let c = [-9.142685, 38.736946];
  let z = 2;
</script>

<button on:click={() => {
    c = [-3.13, 17.73];
    z = 6;
}}>Jump somewhere else</button>
<MapLibre
  style="https://basemaps.cartocdn.com/gl/positron-gl-style/style.json"
  center={c}
  zoom={z}
//...
dimfeld commented 6 months ago

@timadevelop yeah makes sense, it would probably be better to combine both of those into a call into map.easeTo instead.