Skip to content
This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

Merge duplicate plugins #20

Closed
rowanwins opened this issue Apr 12, 2016 · 3 comments
Closed

Merge duplicate plugins #20

rowanwins opened this issue Apr 12, 2016 · 3 comments

Comments

@rowanwins
Copy link

Gday @jgravois

So it turns out we've made almost identical leaflet addins, whoops, not enough googling on my part (although perhaps you should add your addin to the leaflet addin page)!
https://github.com/rowanwins/Leaflet.SvgShapeMarkers

I'm all for merging (slash decommissioning mine) the addins given that I think we're largely trying to solve the same problem in pretty much the same way.

One significant difference worth discussing/thinking about is that rather than creating a new class for every marker type, I instead had an option specifying the type (eg shape: "square"). The reason I went this approach was when I was thinking about how I'd use it I was thinking of a large point dataset loaded as geojson and doing pointToLayer, it seemed a bit more logical to have everything as one

    pointToLayer: function (feature, latlng) {
      return L.shapeMarker(latlng, {
        shape: getShape(feature.properties.myCategory),
        color: 'white'
      })

with your're current approach it would be something more like

    pointToLayer: function (feature, latlng) {
      if (feature.properties.myCategory == "whatever"){
           return L.shapeMarkers.xMarker(latlng, {
               shape: getShape(feature.properties.myCategory),
               color: 'white'
            })
      }
      if (feature.properties.myCategory == "somethingElse"){
            return L.shapeMarkers.squareMarker(latlng, {
                 shape: getShape(feature.properties.myCategory),
                 color: 'white'
            })
     }
}

Obviously you could restructure my example above of your's abit so it's all a bit tidier etc so I guess I'm neither here nor there.

Generally speaking I think your plugin looks a bit more robust, I hadn't thought about canvas at all, it was just the result of a couple of hours of effort yesterday.

Anyway just thought I'd raise the flag
Cheers
Rowan

@jgravois
Copy link
Contributor

with regard to publicity, are you talking about adding a link to our own plugin in http://esri.github.io/esri-leaflet/? if so, i'm happy to do that.

if you're talking about Leaflet's own plugin page, it'd be helpful if you could send a PR yourself to propose that the maintainers add a link.

I'm all for merging (slash decommissioning mine)...

i don't think there is any harm in keeping both versions around with different signatures. alternatively if you're interested in PRing your own change here (and updating the doc, tests etc., along with esri-leaflet-renderers to handle the breaking change), i'm happy to review and merge because i do like your signature more.

to be perfectly honest, i don't have the inclination to do it all myself.

@rowanwins
Copy link
Author

Gday @jgravois

Perhaps I'll keep mine separate for a while given now that I've already got it up and running.

Certainly understand the desire to not to it all by one's self and I super-appreciate the work you do with all the various plugins you maintain! If I can make useful contributions then I'll certainly try :)

Cheers
Rowan

@jgravois
Copy link
Contributor

If I can make useful contributions then I'll certainly try :)

you've already made plenty. 😛

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants