angular-ui / ui-leaflet

AngularJS directive to embed an interact with maps managed by Leaflet library
http://angular-ui.github.io/ui-leaflet
Other
314 stars 134 forks source link

Unixify Plugins / Leaflet Extensions #176

Open nmccready opened 9 years ago

nmccready commented 9 years ago

The current implementation of supporting plugins is very monolithic and puts a lot of complexity into the core of ui-leaflet. Myself and @jessertaylor are proposing we de-scope ui-leaflet and come up with a more plugin framework approach so that we can decorate directives, services, and or factories.

This would put more responsibility on individual repos looking to extend the features of what we provide in house.

Pros:

Cons:

nmccready commented 9 years ago

If anyone knows existing plugin frameworks (javascript) that help iterate over prototype functionality to extend classes .. let me know. Otherwise I am thinking of writing a basic ES6 lib that does this. Then I plan on having a angularjs wrapper to wrap this lib.

nmccready commented 9 years ago

@jessertaylor @elesdoar @zacronos @simison open to comments for sure!

simison commented 9 years ago

See also https://github.com/Leaflet/Leaflet/blob/master/PLUGIN-GUIDE.md for inspiration.

elesdoar commented 9 years ago

Yes, I agree,

nmccready commented 9 years ago

Tagging @sindresorhus to see if he has any suggestions as I consider him a expert. I would love to know if he has any suggestions before I one off something.

simison commented 9 years ago

Also @mourner

TL;DR — we're looking for a way to let people easily extend angular-leaflet with plugins. Any good examples / libraries out there?

j-r-t commented 9 years ago

The way ui-grid have done it may be a good example? https://github.com/angular-ui/ui-grid/tree/master/src/features

"Plugin architecture allows you to use only the features you need"

However, instead of the plugins being in the same repo (like ui-grid), maybe they should be in their own and available via bower, for example

bower install ui-leaflet-label

This would be a fork of https://github.com/Leaflet/Leaflet.label, but with a separate angular part as part of the plugin which is the part which interacts with ui-leaflet (thus keeping the bloat down and making sure the core of ui-leaflet is kept as simple and clean as possible). This may also mean we could keep the plugins in sync (I think) with the original if they're still being maintained, else we can maintain the fork.

eczajk1 commented 9 years ago

I agree that the plugins should not be in the repo, but available from other repos. That way issues like #117 could be solved by writing a plugin for that API - instead of modifying ui-leaflet.

nmccready commented 9 years ago

@jessertaylor I agree with you, I am just trying to think about the code/internal implementation. At a high high level I think we all agree.

j-r-t commented 9 years ago

Yes, I did give ui-grid as an example but have not had time to give it a proper look over, what do you think?

nmccready commented 9 years ago

Looking at it now; conceptually it looks good; but with all the features being in the repo I do not agree with that. We can have a few features locally as examples but I think the majority should be totally separate. Where we load from a specified angular service. IE it is the users choice on how to fill up that service.

nmccready commented 9 years ago

@jessertaylor your suggested way via ui-grid looks pretty good. Where I think most of the hook in to the core is via https://github.com/angular-ui/ui-grid/blob/master/src/features/pinning/js/pinning.js#L35 .

So in marker label case.

So basically we would extend our leaflet markers options which right now is "markers". If on registering a new option definition that has an identical signature; like addMarkers basically push that functionality in an execution chain to be called with the same args.

nmccready commented 9 years ago

Here is an example of a completely standalone directive which extends our base leaflet directive. This can be in a separate repo:

directiveName = 'lfDraw'
angular.module('ui-leaflet')
.config ($provide, nemDebugProvider) ->
  debug = nemDebugProvider.debug
  debug.enable("config:*")
  log = debug("config:#{directiveName}")

  $provide.decorator 'leafletDirective', ($delegate) ->
    directive = $delegate[0]
    directive.scope[directiveName] = '=?' #the way it should be
    #the above should derrive $$isolateBindings this without the below hack
    #workaround from http://plnkr.co/edit/CRxhX6?p=preview
    directive.$$isolateBindings[directiveName] =
      attrName: directiveName
      mode: '='
      optional: true

    # log directive.scope
    log $delegate
    $delegate

.directive directiveName, (leafletLogger, leafletData, leafletHelpers, leafletIterators) ->
  $log = leafletLogger
  isDefined = leafletHelpers.isDefined
  errorHeader = leafletHelpers.errorHeader

  restrict: 'A'
  scope: false
  replace: false
  require: ['leaflet']

  link: (scope, element, attrs, controller) ->
    mapController = controller[0]
    leafletScope = mapController.getLeafletScope()
    _featureGroup = undefined

    leafletScope.$watchCollection directiveName, ->
      #TODO Watch options and recreate control
      options = leafletScope[directiveName] or {}

      mapController.getMap().then (map) ->
        if _featureGroup
          map.removeLayer _featureGroup

        if !isDefined L.Control.Draw
          $log.error "#{errorHeader} Leaflet.Draw is not loaded as a plugin."
          return

        if !isDefined(options.edit) or !isDeinfed(options.edit.featureGroup)
          angular.extend options,
            edit:
              featureGroup: new L.FeatureGroup()

        _featureGroup = options.edit.featureGroup
        map.addLayer(_featureGroup)
        drawControl = new L.Control.Draw options

        map.addControl(drawControl)

        #TODO Add in event handling via extending leafletEvents for leafletDrawEvents

This is not ready, but it shows an idea. I will keep flushing this out in house before I cut a repo.

nmccready commented 9 years ago

Damn in testing this, extending scope does not get picked up due to this bug https://github.com/angular/angular.js/issues/10149

nmccready commented 9 years ago

Modified code above to make it work via hack noted in the url previous to this..

elesdoar commented 9 years ago

Please read: http://www.bennadel.com/blog/2870-using-module-decorator-in-angularjs-1-4.htm

nmccready commented 9 years ago

I already did, but using a module decorator should not be necessary.

elesdoar commented 9 years ago

Other good links for read:

http://www.codeproject.com/Articles/998594/Extending-Angular-Directives https://github.com/angular/angular.js/wiki/Understanding-Directives#extending-directives