Closed rognstad closed 7 years ago
I went with option 3. I'm happy to change it up if you feel strongly about it.
I updated the existing tests and added some new ones.
I also added gmMarkersAdded and gmMarkersRemoved events that are emitted with a list of the added or removed google.maps.Markers. That's really convenient for a marker clustering directive, which I'll add shortly.
Awesome thanks for taking the time to put together a PR.
I like the general idea, just have a couple questions/comments, to do with the angulargmMapController:
Also out of curiosity what is your use case for needing to cluster only certain markers on a map? I don't know if I have ever seen a map with only some markers clustered before. Just saying because as a feature clustering would be much simpler to implement at the map level, i.e. for all markers on a map, and would seem to cover most use cases.
Thanks for the feedback. In terms of angulargmMapController:
marker
is hardly a burden, and there's value in keeping function signatures consistent.I agree that it's typical to cluster all markers on a map. It's also pretty common to only have one set of markers on map. I think having control over which groups of markers cluster could be useful in any situation where someone uses multiple gmMarkers directives. In my particular case, I show vehicles that I want to cluster and places (like business locations) that I don't want to cluster. Including the places in the car clusters would make vehicle density tougher to judge at a glance.
I'll make some adjustments based on your suggestions. I have some more in-progress changes locally that add a clusterer directive. I have it working for my situation, but I need to add the ability to customize the clusterer configuration, and I need to jump through some hoops since the clusterer directive can't access the gmMarkers scope. I had to shift focus to something else last week, but I'll get back to this in the next couple days (hopefully today).
Cool. Again thanks for putting together a PR!
In summary,
angulargmMapController: + getElementsByScopeId + setObjectsNameForScopeId/getObjectsNameForScopeId (or whatever you decide to name it) (Also, since we store and lookup elements based on type + scopeId it may make sense to do the same for objectsName even if it's not strictly necessary.)
angulargmShape: + set objectsName for scopeId on creation + gmShapesAdded/gmShapesRemoved (Would you please add these to the docs at least for gmMarker? I can copy/paste to the other shape directives.)
Sounds good to me.
Okay, I think I did a reasonable job of incorporating your suggested changes, and I added a gmMarkerClusterer directive.
I'm not really sure what tests I should write for gmMarkerClusterer. There isn't much there. I'd be happy to write some, though. In general, I'm not sure if more tests are needed.
I don't like the way I'm storing the clusterer options on angulargmMapController, but I haven't thought of a prettier way to do it. I like having the gm-marker-clusterer attribute on the same element where the to-be-clustered gm-markers are. That means, though, that gm-marker-clusterer can't have its own scope/attributes, and it can't get at the gmMarker's transcluded scope.
One other thing I was thinking about in terms of gmShapesAdded/gmShapesRemoved: I don't really expect these events to be of much use outside of the clusterer context (or else they probably woudl already have been requested). What would you think about calling event.stopPropagation() on them in angulargmMapController so they don't bubble up further?
I think this is basically a feature-complete solution to #24 and #87 at this point. Just let me know if there's more you'd like me to clean up.
Ok it's looking good to me but I really would have liked to look at the marker clusterer directive separately in a different PR. Is there any way you can pull them apart into separate PRs?
On a side note what do you think about completely replacing scopeId with objectsName for identifying groups of elements? I haven't been able to think of any problems or edge cases with that so far but my head isn't as in the code as yours. Not asking you to make this change just wanted to see what you thought.
Supporting marker clustering was always my end goal, so I hadn't thought about separating the clusterer directive from the preliminary work necessary to support it. I see how the title of my pull request and branch name didn't convey that, though. I can split this up however you'd like. The broad strokes of the division you requested are pretty clear, but I'm not sure about some of the details.
This is what I'm thinking will stay here:
Here's what moves to the new pull request:
Please let me know if that's what you're looking for or if you'd accept a less granular split.
I don't have strong opinions about switching to objectsName to track all groups of elements internally.
I can maybe imagine someone wanting to use a single set of objects to define markers and circles, but assuming the tracking would include objectsType (like it does currently with scopeId), that wouldn't be an issue. Even if AngularGM didn't handle it, using a single set of objects for multiple sets of elements is a pretty odd use case, and the developer using the directives could (and maybe even should) work around it by using two sets of objects.
I don't think there are big drawbacks to keeping the existing focus on scopeId. People who need to access elements by objectsName can do that. The map from objectsName to scopeId doesn't add much complexity. Can you think of any situations where someone would find knowing the id of the transcluded scope useful? If there are uses for that, then I think it should stay. If there aren't, switching to objectsName entirely will be slightly cleaner, but it doesn't really get me fired up. shrug
Yep that division would be perfect, thanks for being flexible.
Your reasoning on scopeId/objectsName makes sense, thanks for weighing in!
I finally got around to reverting all that stuff. Sorry for the delay. Is there anything in particular you want me to do with the new PR for the clusterer?
Just a new PR is fine. Thanks!
I'm been trying to get marker clustering to work and ran into a problem I mentioned on using angular-gm with MarkerClusterer. Basically, because of directive transclusion, it's difficult to get at only the correct set of markers when using multiple gmMarkers directives.
Would you be willing to accept a pull request along these lines to make that easier?
It still needs some work (e.g. tests). The current implementation isn't very testable since the controller tests use angulargmMapController.addElement() while I'm currently only updating the objects-name-to-X associations in angulargmShape.updateElements(). I could go a bunch of ways and would like to know which you'd prefer:
(P.S. Sorry for the two messy commits at the beginning. I pulled from your version into mine when I should have just added it as an upstream remote.)