doublesecretagency / craft-googlemaps

Google Maps plugin for Craft CMS - Maps in minutes. Powered by the Google Maps API.
https://plugins.doublesecretagency.com/google-maps/
Other
10 stars 9 forks source link

Provide a way to remove markers completely and reset the bounds #39

Closed MoritzLost closed 2 years ago

MoritzLost commented 2 years ago

The JavaScript DynamicMap API is lacking a way to remove markers completely, and the existing method to hide markers doesn't work properly.

I'm building a sort of store locator where users can enter a location and find the stores closest to them as markers on a map. When a new location is entered, I want to remove the markers for the previous location and add new ones for the new location. However, the map is lacking in methods to remove / hide existing markers, which makes this difficult. In particular:

hideMarker() doesn't adjust the bounds of the map used for fit()

When you add a marker, the map's bounds are extended to include that marker. However, when you hide a marker, the bounds aren't adjusted. So when you remove markers in different locations, using the fit() methods gives you a huge area which includes all markers that were ever created, even existing ones.

There's no method to remove all markers

There should be a utility method to remove all markers, or an option for the markers() method to replace the previous ones. Right now I'm doing that manually, but it's a pretty involved and I have to access the internal state of the DynamicMap object, which I really don't like …

const newMarkers = [{ lat: '…', lng: '…', }, { lat: '…', lng: '…', }];
// hide existing markers
Object.values(map._markers).forEach(marker => marker.setMap(null));
// reset the markers array
map._markers = [];
// reset the map bounds
map._bounds = new window.google.maps.LatLngBounds();
// update map with new markers
map.markers(newMarkers);
map.fit();
lindseydiloreto commented 2 years ago

Hi there, thanks for the feedback. I've addressed both of these concerns by enhancing their respective features.

fit()

The internal behavior of fit has been improved significantly. It will now accurately reflect all visible markers.

hideMarker('*')

You can now hide all markers on a map using an asterisk (*) for the markerId parameter.

Similarly, you can now use an asterisk to specify all markers in showMarker, or all KML layers in hideKml / showKml.


These changes will be formally released in v4.1.0.

In the meantime, you can update your composer.json and run composer update to get the latest dev version...

"doublesecretagency/craft-googlemaps": "dev-v4-dev"
MoritzLost commented 2 years ago

@lindseydiloreto Thanks for the update! I just tested the new method, works fine for the most part. But it doesn't play nice with the cluster option. When clustering is active, I'm seeing two problems:

If I reset clear the markers in the cluster manually the second problem goes away:

const cluster = map.getMarkerCluster();
if (cluster) cluster.clearMarkers();

Could you take another look? Thanks!

lindseydiloreto commented 2 years ago

Ah, bummer. I'll take another look at these with clustering enabled. 👍

lindseydiloreto commented 2 years ago

So we've hit an interesting twist...

Google has completely replaced the underlying MarkerClusterer library. It's a minor pain, because some people will have to adjust their code. But long-term, it's a good thing... nice to finally see Google taking the clustering library seriously.

It certainly has an impact on how this plugin handles clustering, though overall it seems to be a positive change. I've already updated the library internally, feel free to pull the latest to see the difference...

"doublesecretagency/craft-googlemaps": "dev-v4-dev"

Only update now if you are doing basic marker clustering. If you need advanced clustering, you'll need to wait a little longer for me to iron that part out. I'll also need to update the documentation to describe the new approach.

After updating the internal marker clustering library, I was able to fix all of the issues related to fit, hideMarker, and any other odd clustering behavior. I haven't published any updated documentation, but feel free to update to the latest dev version and give it another shot.

MoritzLost commented 2 years ago

@lindseydiloreto Thanks for your efforts! I've tested the update and it works well. Incidentally, this update also seems to resolve my problem regarding the cluster assets in #40 since the icons now appear to be embedded into the markercluster library.

One minor bug I've encountered: The first time I'm using map.fit(), the map zooms all the way to the highest zoom level (close to the ground) so the markers aren't visible at all if they're a bit spread out. Not sure why this happens, the second time the map is populated with markers it works fine. Maybe something in the cluster is initialized asynchronously?

Here's the relevant part of my map update function:

// hide existing markers
map.hideMarker('*');

// add new markers
const markers = [/** ... */];
markers.forEach(marker => map.marker(
    marker,
    { /** additional options for info windows */ }
));

console.log(map._getBounds());

// fit the map to the new markers
map.fit();

The first time this is executed, the bounds logged to the console look like they only include the center point between the added markers:

bounds-1

The second time I run my map update function (which hides the previous markers), the map centers and zooms correctly, and the bounds look normal:

bounds-2

Even if I delay the map.fit() call with setTimeout() the first execution this has this error. Any ideas? ^^

lindseydiloreto commented 2 years ago

Incidentally, this update also seems to resolve my problem regarding the cluster assets in #40 since the icons now appear to be embedded into the markercluster library.

Excellent point! The new clustering library uses SVGs to generate the default icons, instead of PNGs like the previous clustering library. Thus, the entire concept of _defaultClusterPath becomes moot.

Thanks for pointing that out, I'll close #40. 👍


Regarding the lingering issue with .fit(), would you mind hitting me up on Craft Discord? It looks like you're doing some very creative things with the map, I'd love to get the "big picture" view of it all.

Feel free to DM me on Discord!

lindseydiloreto commented 2 years ago

After discussing the details via DM, we were able to tighten up any loose ends.

All of these changes will be formally released in the upcoming v4.1.