Open rognstad opened 6 years ago
dylanfprice
commented
6 years ago Hey thanks for the PR! It does seem like there are a lot of special cases--have you considered what it would look like to implement a generic "shape constructor" override? I.e just like there is a global default markerConstructor, polylineConstructor, and circleContructor, there could be a per-directive override for each shape as well.
rognstad
commented
6 years ago I could do something like that.
Do you know of some examples of custom polyline and circle constructors? I would like to take a look at how they're used. I think it would be pretty easy to add support for per-directive custom polyline and circle constructors assuming they extend google.maps.Polyline and google.maps.Circle.
I didn't do it initially because I wasn't aware of much demand for customizing those shapes, and I had a harder time thinking of situations where someone would want to use multiple constructors on a single map. I may just be too focused on what I do in my app, but it's easy for me to imagine having a bunch of different types of markers on a map and wanting sets of them to be styled radically differently. At that point, I know it's fairly common to extend OverlayView to act as custom markers in order to put arbitrary HTML on the map:
I feel like Google providers more sufficient styling options for polylines and circles than markers. I did some searching this morning to try to validate that impression. There is a neat Android project (maybe dead?) that allows cool stuff: https://github.com/antoniocarlon/richmaps The official Android Maps API added additional customization options this year (https://developers.google.com/maps/documentation/android-api/releases#february_15_2017). There's definitely discussion about gradient polylines (which seems to exist in iOS and has an open feature request for Android), though.
I didn't really find much related to the JS API (though maybe "custom" isn't the right term to search for). Most of the questions (e.g. https://stackoverflow.com/questions/28430269/how-to-use-custom-icon-for-polyline and https://stackoverflow.com/questions/20420872/drawing-a-custom-circle-on-a-google-map) are solvable without using custom constructors.
Anyway, this is a pretty roundabout way of saying that even if I can't foresee how it will be used, it shouldn't be much work to add support for it if you want me to do that.
dylanfprice
commented
6 years ago Thanks for thinking of use cases first. I think my point is that coming from an implementation perspective the code is factored to try and make it easy to do things for all shapes at once (whether the specific way it's factored achieves that nicely or not is another question :smile: ). I'd argue that the implementation would be cleaner if you did it for all shapes, even if the only use cases are marker use cases. You could even implement it generically in angulargmShape but only enable it when a custom constructor is passed in, allowing it to easily be turned "on" for markers alone.
Do you think gmMarkerConstructor, gmCircleConstructor, and gmPolylineConstructor are good or should all three directives take an attribute with the same name (gmShapeConstructor, gmConstructor, gmCustomConstructor, or something else)?
Given the precedent of gm-marker-options, gm-circle-options, gm-polyline-options, you could follow a similar pattern for custom constructors. But, I don't feel strongly.
Hey, here's another PR!
In my app, I have a single gmMap with multiple gmMarkers directives. I want to use a custom marker constructor (based on OverlayView) for only one of those sets of markers, so setting angulargmDefaults.markerConstructor doesn't really accomplish what I want.
I extended gmMarkers to accept a custom marker constructor function passed as gmMarkerConstructor. It will apply only to the markers created by that gmMarkers directive.
Here's a basic Plunker: http://plnkr.co/edit/ieC6HIkJMulII5YHMl6A?p=preview
I think this is decent, but there are a few things I want to mention:
gmMarkersSpec.js:
I'm not super familiar with jasmine and had a lot of trouble here. I added a whole suite of tests for gmMarkerConstructor because my custom marker constructor wasn't getting called at all when I tried follow the pattern used in the "requires the gmPosition attribute" test. It seemed like I needed to do all the element setup, compilation, and controller work before the it() call.
I also struggled to successfully mock my constructor and its prototype methods. The expectations in "is used when a custom marker constructor is provided" kind of beat around the bush because of that. Basically, as soon as I tried to spy on my constructor, it clobbered all my instance methods, and I couldn't seem to get them back.
angulargmShape.js
I don't like having to inject and use attrs.gmMarkerConstructor to test whether someone passed a constructor, but scope.gmMarkerConstructor exists and is a function no matter what thanks to Angular.
I don't like having two separate calls to controller.addElement in _addNewElements, but using a single one and passing an undefined custom constructor as the last argument would have required a major rework of tests that check what arguments are passed to the controller's addElement function. It wouldn't be so bad to update all the tests once, but it seemed like it would create a fair bit of frustration with any future updates/tests.
Requiring the custom constructor to have a "getDomElement" method and the test to verify that it returns a DOM node seem a little cludgy, but I think it is necessary since OverlayView events need to use addDomListener rather than just addListener, and that requires passing the appropriate DOM node (not the marker) as the object when creating the listener.
I'm happy to make changes if you have suggestions about any of this stuff.
Thanks for considering the PR and all your work on AngularGM!