dylanfprice / angular-gm

AngularJS Google Maps Directives
MIT License
197 stars 47 forks source link

[Polylines] Init polylines directive #9

Closed cboden closed 11 years ago

cboden commented 11 years ago

Add Polylines to AngularGM:

<gm-map gm-map-id="'myMap'" gm-map-options="mapOptions">
        <gm-polylines gm-objects="polylines"
                      gm-get-path="object.path"
                      gm-get-polyline-options="getPolylineOptions(object)">
        </gm-polylines>
</gm-map>
$scope.mapOptions = {yada: 'yada'};

$scope.getPolylineOptions = function(line) {
    return {
        strokeColor: '#F00',
        strokeOpacity: 1,
        strokeWeight: 2
    }
}

$scope.polylines = [
    {
        path: [
            {lat: 37.772323, lng: -122.214897}
            {lat: 21.291982, lng:-157.821856},
            {lat: -18.142599, lng: 178.431},
            {lat: -27.46758, lng: 153.027892}
        ],
        otherDomain: 'specific logic',
        hello: 'world'
    }
];

I tried to follow your pattern with how you did the markers. There may be a better way than to have path as an array of lat/lng objects. Would appreciate some feedback while I continue to work on this.

dylanfprice commented 11 years ago

Awesome this is looking great!

Path as an array of lat/lng objects seems fine to me. Another option would be to start using GeoJSON formats:

{ "type": "Point", "coordinates": [-75, 21.9] } // for gm-get-lat-lng in gm-markers
{ "type": "LineString", "coordinates": [[-75, 21.9], [-75.4, 22.7], [-76.5, 23.4]] } // for gm-get-path in gm-polyline

But I always find the lng,lat ordering confusing, especially since google maps does it the other way.

I'll wait until you've fleshed it out a bit more but it seems like we'll want to extract common logic between markers, polylines, and potential other shapes (polygon, rectangle, circle).

As a heads up, I am pretty much decided on converting the docs to use docular, so don't bother copying the documentation format of gmMarkers until I make the change.

Also, just as a side note you should be able to just do:

gm-get-polyline-options="{ strokeColor: '#F00', strokeOpacity: 1, strokeWeight: 2 }"

since you aren't changing your options based on the object. This probably isn't clear in my docs so maybe I'll adjust the wording somehow.

Thanks for the work so far!

cboden commented 11 years ago

Thanks for the feedback!

Another option would be to start using GeoJSON formats

Cool. Standards are always a good option!

I'll wait until you've fleshed it out a bit more but it seems like we'll want to extract common logic between markers, polylines, and potential other shapes (polygon, rectangle, circle).

Yup...so much of this PR is copy/paste. There's definitely a lot that can be refactored for reuse that will also apply to the other directives when they're created. For this iteration I'll leave it the way it is (once functional) and we/you can decide the best approach to reuse code after.

Also, just as a side note you should be able to just do

Very cool. That makes total sense. In my practical app I actually have more domain object in the lines and the colour is decided by a combination of values in the passed object, but it's good to know I can just use an object in my template for ease of use.

cboden commented 11 years ago

ok, I think this is ready for a review. :)

dylanfprice commented 11 years ago

Okay Chris, sorry to be slow but I'll be able to take a look at this early next week.

cboden commented 11 years ago

Thanks for the update Dylan. I understand how difficult it can be to manage an OS lib in your spare time. We're currently using my fork in our project until this gets merged/refactored into your master.

cboden commented 11 years ago

Rebased on top of master (2.0)

dylanfprice commented 11 years ago

Cool I think this is pretty much ready. I made a few comments on the diff so let me know your thoughts on those.

The documentation has been switched, so you can pretty much copy/paste what's in gmMarkers.

I've been thinking about how best to refactor all this and there's defintely some things to think about. I recorded some of these things below so I can keep track, but it would also be awesome if you have any thoughts or feedback and want to chime in.

gm-polyline vs gm-polylines

The much simpler way to implement polylines would be a gm-polyline directive. You could achieve the same thing we have now with ng-repeat. The reason I have gm-markers is because I want to support 1000s of markers without creating 1000s of extra dom nodes and having ng-repeat recalculate and slow things down. However, I'm not sure the same use case exists for polylines. Do people ever want 100s or 1000s of polylines?

gm-get-lat-lng and gm-get-path

The purpose of these is to convert from your model's representation into something we know how to feed to the google maps api. The best would be to make people convert from their model to the google object, i.e. gm-get-lat-lng would return a google.maps.LatLng. However, angular won't let us do gm-get-lat-lng="new google.maps.LatLng(object.lat, object.lng)" and I don't want to force unnecessary scope functions for what's usually a really simple conversion.

But with polylines there a couple good reasons to let people return an MVCArray<LatLng> or Array<LatLng> directly.

So I think I want to eventually support both the object literal and the google object. To do this, I'll need a better way to identify objects. Maybe add something like a gm-get-id attribute?

cboden commented 11 years ago

Thanks for the review. I've fixed the bugs you found!

Hmmm how can we allow triggering of events on a polyline? We may need a better way to identify a polyline than just a hash of its path. For now we should just not support this and remove the gmEvents attribute.

I'm not quite sure, I didn't like my concatenation of coords, perhaps the user could provide a unique id or have one auto generated if not passed? That option should make it easy to implement events.

Do people ever want 100s or 1000s of polylines?

I probably will have this many in my project. As well, having the directive represented as a collection unifies the API between all the objects put on the map.

So I think I want to eventually support both the object literal and the google object. To do this, I'll need a better way to identify objects. Maybe add something like a gm-get-id attribute?

Sounds like a good idea, as long as users still have the ability to put raw objects in the template.