tombatossals / angular-leaflet-directive

AngularJS directive to embed an interact with maps managed by Leaflet library
http://tombatossals.github.io/angular-leaflet-directive
MIT License
1.5k stars 637 forks source link

improvements #660

Closed nmccready closed 9 years ago

nmccready commented 9 years ago

I am the main contributor to angular-google-maps and myself and my company(realtymaps) are seriously considering leaflet and mapbox. Therefore we were hoping to use this directive and make improvements.

However we wanted to run by some ideas before we begin working on them.

Markers:

The data structure accepted seems flat and we would like an easier more efficient way to remove and add data without having to check the specific layer name for each marker.

Thus, supporting the following structures alternatives proposed:

markers = 
    layer1:
        options: {
           color: '#fff',
           icon: 'someUrl'
        }
        data: [] // or {} ie same root structure now but no need for internal layer option (unless slapped on at creation)
    layer2:
     //.. identical but different options and or data
<leaflet markers-options="{ layer1: {color: '#fff', icon:'someUrl'}, layer2: {...}, ...}"></leaflet> 

Where markers-options evaulates the expression as an object, function .. whatever to get the options. Which would yield the same data structure for markers now but setting the options for all markers according to their layer.

<leaflet markers-nested="true | or parsed expression"  markers="someNestedMarkers"></leaflet>" 

Where the the controller:

var someNestedMarkers = {
  layer1:{
    1:{
       lat: 45, lng:100
     }, ....
  },
  bikes:{
    1:{
       lat: 45, lng:100
     }
  }
}

The above structure makes it much easier to remove the data from the correct layer without having to iterate through each marker to figure out what to delete and then what to add.

nmccready commented 9 years ago

Also I would like to add a multpolygon directive as well as having markers and multipolygon support parsing data as geojson (geoJson -> LatLng). This is easy code which can be ported from ui-gmap easily.

nmccready commented 9 years ago

Keep in mind we are just trying to open a discussion here on the best approach/ way forward.

zacronos commented 9 years ago

Hi folks, I work with @nmccready at RealtyMaps, so I would be contributing here as well if we switch from angular-google-maps to angular-leaflet-directive. After some discussion with @nmccready, I have an alternative to the proposal above.

The following could be implemented as 2 separate features/PRs, and would be backward-compatible. To be more clear, we are offering to do the work, but we don't want to put in the time if the PR will just be rejected, so we'd like to at least get approval for the general approach of the API enhancements before work begins on our end.

Feature 1: per-layer marker defaults

A new directive, markers-defaults, would work basically as marker-options described above; it would be an object mapping layer names to options that should be applied by default for all markers on that layer. If an option is defined in an given marker object as well as in the defaults for that layer, the marker object's value would override.

This would be a more convenient way of handling options for different types of markers.

Feature 2: nested marker containers

Right now, markers can only be defined as a single container ([] or {}) of markers. If markers come from more than 1 data source though, this requires manually merging the lists/maps of objects together for angular-leaflet.

Instead, a new directive could be added, markers-nested=<true | false>, which if true causes markers to be interpreted as a container of containers of markers. This makes it easy for the application to add or remove multiple markers at a time, and probably wouldn't require much more than adding a nested loop to the existing angular-leaflet code.

Feature 2: alternative implementations

Or, instead of a directive to turn this new behavior on, instead a new directive could have this behavior; perhaps markers-multi is always a container of container of markers, where markers is always just a container of markers. In this case, it would probably be an error to use both markers and markers-multi in the same angular-leaflet tag, however.

Or, no new directive would be needed if we make this a breaking change to the API and always use the container of containers approach... but I'm guessing you'll want to keep backward compatibility if possible, so I mention the idea here only for completeness.

tombatossals commented 9 years ago

Hi @nmccready @zacronos, guys, I know you for your superb work in angular-google-maps. Your proposals are received like fresh air to this directive, of course all your PR will be accepted.

I think it would be interesting to keep backward compatibility and mantain the flat "markers" directive as it is now, as a simple way to show markers in a map, but I understand your idea of nested markers, grouped by layers, and I like it. It could add some advanced funcionality to the directive.

The two proposals seems good to me, the "markers-nested=true|false" or the new "markers-multi" directive, please choose by yourself, any of those will be good.

And feel free to send any PR or any suggestion of change to the code. Thanks for your contribution!

nmccready commented 9 years ago

@tombatossals what is your defacto branching scheme? IE which branch is the dev branch (to be next release) is it master?

nmccready commented 9 years ago

To make my life easier I am probably going to PR some grunt stuff to start out with to get explicit versioning (version in the dist files) and sourcemaps into the project.

nmccready commented 9 years ago

661

nmccready commented 9 years ago

I think markers-nested is more explicit as the way markers is now it handles multi and layers. markers-nested indicates something more.

nmccready commented 9 years ago

@tombatossals trying to decide if I even need multipolygon. If I can get events of specific polygons from geojson then I wont need multipolygon.

Here is my example I am playing with.

http://jsfiddle.net/nmccready/3avtpv8b/12/

Does leaflet support events to geojson objects?

nmccready commented 9 years ago

nvm I think I found it.. sweet https://github.com/tombatossals/angular-leaflet-directive/blob/1c6d17514623a8bc91546c619c1b73af33ba81a4/src/directives/geojson.js#L30-L54

nmccready commented 9 years ago

http://jsfiddle.net/nmccready/3avtpv8b/13/

Sick it is working now nice.

I will then just tackle the nesting.

tombatossals commented 9 years ago

Hey guys, maybe is the perfect time to work with a more optimal git branching & publish schema. Are you confident with git flow? Basically the master branch is the stable branch and any new feature is added in new branches.

I can add you two as main developers of the project, so advances will be more agile and dynamic.

Please tell me what you think about it.

nmccready commented 9 years ago

Yes I have done gitflow before. Only issue I have seen (which is why I moved off it) is that the develop branch is kinda generic. Also the idea of master always being stable is kinda tough especially since most devs will PR against master. This is where tags become more important.

The current setup on ui-gmap is the following:

master - current active (next planned release) really just a clone of 2.1.X
2.1.X - explicit relying on tags to 2.1.X where tags are considered stable and 2.1.X itself is possibly in-stable
2.0.X  - explicit relying on tags to 2.0.X where tags are considered stable and 2.0.X itself is possibly in-stable
''''''' and so on

The idea of having master target the next planned release is to force contributors to start thinking and move on to the new version. I did this because getting off 2.0.X has been rather difficult.

nmccready commented 9 years ago

@tombatossals what is your gh-pages proccess/workflow? To update the site and point to a new release?

tombatossals commented 9 years ago

You're totally right, like your point of view. So master is a clone of the branch were all the new development is done, and to get an stable release you get a tagged version.

@nmccready I admit I have not workflow in gh-pages, and very basic on the master. This project needs some cleanup and now will be the perfect time with your help, so I will adopt any you think is good enough. :smile:

nmccready commented 9 years ago

@tombatossals do you want to start a gitter or slack chat room public and a private (devs). Might be easier to communicate since I don't have an email or chat for u.

nmccready commented 9 years ago

If you do please do the creation and send the invites. I will invite you to the slack on ui-gmap in the meantime to get email and or other chats.

nmccready commented 9 years ago

PR is here https://github.com/realtymaps/angular-leaflet-directive/pull/4

nmccready commented 9 years ago

669