nuxt-modules / leaflet

A Nuxt module to use Leaflet
https://leaflet.nuxtjs.org/
Apache License 2.0
108 stars 3 forks source link

feat: Add support for Popups and legacy methods with useMarkerCluster #58

Closed tratteo closed 1 month ago

tratteo commented 1 month ago

Hi, sorry for creating an issue for this question but I do not see the discussion page in this repository. I am using the leaflet.markercluster integration to add markers to my map. However, I want to be able to open a LPopup when a Marker is clicked. At the moment, its seems I cannot specify the html content of the marker when passing the markers array to useLMarkerCluster. ❓ Is it possible to achieve the same behaviour in clustered markers as if the markers were added in the traditional way (the following)?

<LMarker :lat-lng="[41.75, -87.65]" draggable>
     <LPopup> Hi! You can drag me around! </LPopup>
</LMarker>

Thanks a lot!

Gugustinette commented 1 month ago

Hi !

No problem, I didn't know I was supposed to activate the Discussions tab lol. I just did so thanks for the reminder !

You should be able to pass the classic Leaflet Marker options in the array like so :

const locations = [
  { name: 'Nantes', lat: 47.218371, lng: -1.553621, options: {
    // Standard Leaflet Marker options
    draggable: true,
    icon: L.icon({
      iconUrl: '/my-icon.png',
      iconSize: [30, 30],
    })
  } },
  ...
]

This is mentioned in the documentation but may not be very clear. I might change that in the future 😄

tratteo commented 1 month ago

Thank you but, does the classic Marker Options allow me to specify a popup html or something?

Gugustinette commented 1 month ago

Ah yeah that's my bad, you can't do that at the moment.

I think we should :

Does that sound good for you ? I can work on it this week I think.

tratteo commented 1 month ago

Yes! That is exactly how I solved it for now. Basically I added the popups with the cluster extension with the usual way and then I inject the popups with this code:

function injectPopups() {
    mapEl.value!.leafletObject.eachLayer(function (layer: any) {
        console.log(layer);
        if (layer instanceof L.Marker && layer.options.title) {
            layer.bindPopup(`<h3>${layer.options.title}</h3>`);
        }
    });
}

One thing to keep in mind is that the function binds the popup only to Markers and not Clusters of course. Hence for the moment I am also checking for zoom change events so that I can update and bind popups to new unclustered markers, tho I do not know if this is the best way in terms of performance, but surely it is working:

mapEl.value!.leafletObject.addEventListener("zoomend", injectPopups);

PS

To be honest I think this method can be considered the "canon" one, I don't think it is necessary to update the package itself. Updating the documentation would be more than enough!

Gugustinette commented 1 month ago

You could add the popup to every marker at a time when creating them with useMarkerCluster, I believe it would be way more efficient and reliable.

Returning the markers would already be a great addition.

tratteo commented 1 month ago

At the moment the clusterization happens with the composable useLMarkerCluster. This composable takes as input a list of MarkerOptions, not Marker, so how can i bind the popup during creation?

Gugustinette commented 1 month ago

Oh you can't now, I was just writing some things to keep in mind when implementing the feature lol

tratteo commented 1 month ago

Ups sorry 🙋. BTW thanks for the fast responses. I might try to make a PR too if you don't have time to look into it. Thanks again m8!

Gugustinette commented 1 month ago

I should have time to do so today/tomorrow 👍

Gugustinette commented 1 month ago

I made a new release that should fix this : https://github.com/nuxt-modules/leaflet/releases/tag/1.2.1

Happy to try it and confirm it works ? @tratteo

tratteo commented 1 month ago

@Gugustinette I just tested and there is only one small problem. I see that now markers created are returned by the composable and that is good. However, if I add a click event listener on the popup in this way:

const res = await useLMarkerCluster({
      leafletObject: mapEl.value!.leafletObject,
      markers: props.clusteredMarkers.map((m) => ({
          name: m.id,
          lat: m.coords[0],
          lng: m.coords[1],
          popup: m.popup,
      })),
  });
  res.markers.forEach((m) => {
      m.getPopup()?.addEventListener("click", () => {
          // Do
      });
  });

the event is not being fired. I see on some threads online, that the events on the popup are called exclusively if the popup is created with the L.DomUtil.create() method, you can check a thread here. I see on the changes you made that the popup is instead created with the L.popup() method (here) and this is surely the cause. Maybe we should consider creating the popup with the L.DomUtil.create() method since the popup parameter is an html content string anyway!

Gugustinette commented 1 month ago

Yup, L.DomUtil.create() works better, however I found that .getContent() is also required to bind a click event on the popup ?

const popupDiv = markers[2].getPopup()?.getContent() as HTMLElement; // Some weird typing to work around here
popupDiv.addEventListener('click', () => {
  console.log("click")
});

Seems like we can't bind a click event to the popup directly, the DOM element inside must be selected somehow. Can I draft a new release with this behavior ?

tratteo commented 1 month ago

I am trying to bind an event listener through the content but the following code is returning me type string and i am not able to bind event. Probably I am doing something wrong?

console.log(typeof (m.getPopup()!.getContent() as HTMLElement)); // > type string
Gugustinette commented 1 month ago

Well I didn't release the new version, so I guess this is normal for now.

Gugustinette commented 1 month ago

Released here : https://github.com/nuxt-modules/leaflet/releases/tag/1.2.2

tratteo commented 1 month ago

Works like a charm! Thank m8 and good job 🤝