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

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

Open
singingwolfboy opened this issue Apr 15, 2024 · 7 comments
Open

Comments

@singingwolfboy
Copy link
Contributor

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
Copy link
Owner

dimfeld commented Apr 16, 2024

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:

  • pro: Flexible since you're basically taking full responsibility for handling these props
  • con: you're basically taking full responsibility for handling these props :)

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

  • pro: it doesn't require you to implement your own reactive logic outside the map to handle these changes
  • pro: easy to handle things like simultaneous changes to center and zoom resulting in a single API call
  • con: perhaps harder to see what changed in the callback (though we could just pass the old values or a set of changed flags to the callback to help with this)
  • con: the applyBounds callback could do something that doesn't result in those compare calls returning true, which might be valid for the use case but could result in weird behavior or the callback being called infinitely.

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

@dimfeld
Copy link
Owner

dimfeld commented Apr 18, 2024

Planning to implement this next week

@singingwolfboy
Copy link
Contributor Author

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
Copy link
Owner

dimfeld commented Apr 18, 2024

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
Copy link
Contributor

larsmaxfield commented May 16, 2024

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
Copy link

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
Copy link
Owner

dimfeld commented May 16, 2024

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

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

No branches or pull requests

4 participants