-
Notifications
You must be signed in to change notification settings - Fork 264
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 multiple carto clients using Google Maps #2152
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end the solution is succint 👍
Just left a comment. Regarding unit tests, test the three methods you modified / created.
if (overlayIndex >= 0) { | ||
this.gmapsMap.overlayMapTypes.removeAt(overlayIndex); | ||
overlays.setAt(overlayIndex, overlays.getAt(overlayIndex)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit strange since we're setting the overlay at the same exact place. I guess that setAt
triggers something internally in Google Maps. If not, this has no much sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's basically a reload, before we were doing remoteAt 0
-> setAt 0
, but just doing setAt
works fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I have reviewed the new changes since ivan did the first CR and they look good.
I've tested the acceptance example as well as builder with gmaps and it seems like all's good in da hood |
Related to: #2132
Description
This PR allows to use multiple carto clients when using Google Maps.
We had a few problems:
GMapsCartoDBLayerGroupView
, and always usingsetAt(0, layer)
which was overriding other layer groups added previously by other clients.GoogleMapsMapType
to themap.overlayMapTypes
array, but checking forGMapsCartoDBLayerGroupView
instances, so we never found them.map.overlayMapTypes.push
and telling our users to do the same, which lead to duplicated map types.map.overlayMapTypes
(in the example) for each layer, which works with Leaflet because it uses an object and internal layer ids, but not with google maps which just uses an array, we should document this somehow, clients have to call it once for each carto client / layer group.I solved it by adding an internal
id
to both,GoogleMapsMapType
andGMapsCartoDBLayerGroupView
, and check thatid
instead of usingindexOf(this)
.It seems to be working, although
map.overlayMapTypes
can contain two different things depending on where it's being used (builder or public api).I've also checked Leaflet, but since it works differently we don't have that problem.