Update oms.coffee to work with AdvancedMarkerElement#181
Conversation
Changes:
* Google didn't provide an equivalent for getVisible in AdvancedMarkerElement; I'm not quite sure how this was intended to be used - whether it was asking "is this marker's location within the bounds of the map" or "is this marker status visible vs. invisible". So I created a new method IsVisibleMarker that returns true if the marker has a non-null map (map == null is how you remove a marker from the map) AND is within the bounds of that maps latitude/longitude boundaries. This works for my purposes, I hope it is the correct definition
* changed getPosition/setPosition to .position property
* changed getZIndex/setZIndex to .zIndex property
* commented out the line "(return i if o is obj) for o, i in arr" in p.arrIndexOf. If I'm understanding Coffee correctly (I'm not!!) this never gets called if arr.indexOf is defined, so this seems to work...
Probably worth noting a few other things I had to do to migrate to AdvancedMarkerElement:
* No longer use defered script include, so I no longer get a callback when it is loaded, so the old code of waiting for both oms and maps to load no longer works. E.g., now I load googlemaps with (g => { var h, a, k, p = "The Google Maps JavaScript API", c = "google", l = "importLibrary", q = "__ib__", m = document, b = window; b = b[c] || (b[c] = {}); var d = b.maps || (b.maps = {}), r = new Set, e = new URLSearchParams, u = () => h || (h = new Promise(async (f, n) => { await (a = m.createElement("script")); e.set("libraries", [...r] + ""); for (k in g) e.set(k.replace(/[A-Z]/g, t => "_" + t[0].toLowerCase()), g[k]); e.set("callback", c + ".maps." + q); a.src = `https://maps.${c}apis.com/maps/api/js?` + e; d[q] = f; a.onerror = () => h = n(Error(p + " could not load.")); a.nonce = m.querySelector("script[nonce]")?.nonce || ""; m.head.append(a) })); d[l] ? console.warn(p + " only loads once. Ignoring:", g) : d[l] = (f, ...n) => r.add(f) && u().then(() => d[l](f, ...n)) })({
key: "...",
v: "weekly",
// Use the 'v' parameter to indicate the version to use (weekly, beta, alpha, etc.).
// Add other bootstrap parameters as needed, using camel case.
});
So now I need to create the overlappingmarkerspiderfier in oogle.maps.event.addListenerOnce(this.gmap, 'idle', function () { ...}
seche
left a comment
seche
left a comment
There was a problem hiding this comment.
Tested and works with the AdvancedMarker Element
|
@seche Will this be merged anytime soon :) |
| continue unless m1.getMap()? and m1.getVisible() # marker not visible: ignore | ||
| continue unless m1.getMap()? and @isVisibleMarker(m1) # marker not visible: ignore | ||
| m1Data = mData[i1] | ||
| continue if m1Data.willSpiderfy # true in the case that we've assessed an earlier marker that was near this one | ||
| for m2, i2 in @markers | ||
| continue if i2 is i1 # markers cannot be near themselves: ignore | ||
| continue unless m2.getMap()? and m2.getVisible() # marker not visible: ignore | ||
| continue unless m2.getMap()? and @isVisibleMarker(m2) # marker not visible: ignore |
There was a problem hiding this comment.
Just a heads up, AdvancedMarkerElement does not have getMap() anymore and this will result in a TypeError: m1.getMap() is not a function
AME does have map instead, I've replaced in my copy m1.getMap() with m1.map and it seems it works fine. (Same for m2.getMap() -> m2.map)
There was a problem hiding this comment.
Super - is there action required on my part for this to be merged, inclusive of the getMap() => map change?
There was a problem hiding this comment.
I'm not a maintainer of OverlappingMarkerSpiderfier, just a dev that needed this plugin too and used it a bit more extensive and uncovered the issues mentioned in my review.
We've been using your changes combined with mine since I've posted this code proposal in our production, we didn't encounter any other issues since so I believe you can add my changes to your Pull Request.
Sorry for the late reply.
|
Hey @ericberman, while reviewing your PR, I'd suggest the following code changes:
You can also review and apply these suggestions locally on your machine. Learn more about GitKraken Code Suggest
Join your team on GitKraken to speed up PR review. |
I don't have access to that gitkraken suggestion |
6 participants
Changes:
Probably worth noting a few other things I had to do to migrate to AdvancedMarkerElement:
https://maps.${c}apis.com/maps/api/js?+ e; d[q] = f; a.onerror = () => h = n(Error(p + " could not load.")); a.nonce = m.querySelector("script[nonce]")?.nonce || ""; m.head.append(a) })); d[l] ? console.warn(p + " only loads once. Ignoring:", g) : d[l] = (f, ...n) => r.add(f) && u().then(() => d[l](f, ...n)) })({key: "...",
v: "weekly",
// Use the 'v' parameter to indicate the version to use (weekly, beta, alpha, etc.).
// Add other bootstrap parameters as needed, using camel case.
});
So now I need to create the overlappingmarkerspiderfier in oogle.maps.event.addListenerOnce(this.gmap, 'idle', function () { ...}