API Cleanup #324

Open
justinmanley opened this Issue Sep 19, 2014 · 7 comments

Comments

Projects
None yet
6 participants
@justinmanley
Member

justinmanley commented Sep 19, 2014

A proposal for API changes that will bring the Leaflet.draw API more in line with the Leaflet API and make it more user-friendly.

  • Deprecate addToolbar and removeToolbar methods of L.Toolbar. Inherit L.Toolbar from L.Control, instead of L.Class, and move the code from addToolbar and removeToolbar into onAdd and onRemove methods (in line with the rest of the Leaflet API).
  • Add an control.addTo(map) alias for map.addControl(control).
  • Rename L.Control.Draw to L.ToolbarGroup, since that's what it is - a group of toolbars. This will bring it in line with other leaflet aggregator classes (L.LayerGroup, L.FeatureGroup).
  • It's currently very difficult to customize the toolbars that are passed in to L.Control.Draw. The only way to do right now is to extend L.Control.Draw and override (read: copy-and-paste with small modifications) the initialize method. Not very DRY. This could be fixed in two ways:
    1. Break up the code for L.ToolbarGroup into smaller methods and provide a private method _addToolbars so that folks can easily extend L.ToolbarGroup with custom toolbars.
    2. Have L.ToolbarGroup take an array of L.Toolbar objects.

Chime in if you think Leaflet.draw would benefit from these changes! Also would love to hear if there are good reasons that I've missed for the oddities and contradictions of Leaflet.draw that I've outlined above.

@mourner

This comment has been minimized.

Show comment
Hide comment
@mourner

mourner Sep 19, 2014

Member

Sounds pretty good to me 👍

Member

mourner commented Sep 19, 2014

Sounds pretty good to me 👍

@justinmanley

This comment has been minimized.

Show comment
Hide comment
@justinmanley

justinmanley Sep 30, 2014

Member

I'm working on this in manleyjster/Leaflet.Toolbar. (I figured it was general enough to merit its own plugin).

Will post an update when I have a working version which fully reproduces existing functionality.

Member

justinmanley commented Sep 30, 2014

I'm working on this in manleyjster/Leaflet.Toolbar. (I figured it was general enough to merit its own plugin).

Will post an update when I have a working version which fully reproduces existing functionality.

@jacobtoye

This comment has been minimized.

Show comment
Hide comment
@jacobtoye

jacobtoye Oct 3, 2014

Member

I think this is a great idea. Once ready Leaflet.draw can replace it's toolbar implementation with your plugin.

Member

jacobtoye commented Oct 3, 2014

I think this is a great idea. Once ready Leaflet.draw can replace it's toolbar implementation with your plugin.

@justinmanley

This comment has been minimized.

Show comment
Hide comment
@justinmanley

justinmanley Jan 6, 2015

Member

Hey folks - Leaflet.Toolbar is ready - at the very least, it's ready for us to have a discussion about it. I submitted a PR (#354), interested to hear your thoughts!

Member

justinmanley commented Jan 6, 2015

Hey folks - Leaflet.Toolbar is ready - at the very least, it's ready for us to have a discussion about it. I submitted a PR (#354), interested to hear your thoughts!

@pztrick

This comment has been minimized.

Show comment
Hide comment
@pztrick

pztrick Jan 10, 2015

Contributor

👍

I'm already looking forward to rolling some custom toolbars in my projects with Leaflet.Toolbar, and will probably wait for the next Leaflet.Draw release that ships with it.

Contributor

pztrick commented Jan 10, 2015

👍

I'm already looking forward to rolling some custom toolbars in my projects with Leaflet.Toolbar, and will probably wait for the next Leaflet.Draw release that ships with it.

@kyletolle

This comment has been minimized.

Show comment
Hide comment
@kyletolle

kyletolle Jan 16, 2015

Contributor

I'm also looking forward to Leaflet.Toolbar being used in Leaflet.draw, primarily for the popup controls that can be added to individual features. It's great to see all the work done to make that happen! Thanks @manleyjster! This will enable a much more intuitive editing experience in my application.

Contributor

kyletolle commented Jan 16, 2015

I'm also looking forward to Leaflet.Toolbar being used in Leaflet.draw, primarily for the popup controls that can be added to individual features. It's great to see all the work done to make that happen! Thanks @manleyjster! This will enable a much more intuitive editing experience in my application.

@fnicollet

This comment has been minimized.

Show comment
Hide comment
@fnicollet

fnicollet Feb 6, 2015

Contributor

Great work @manleyjster , this is a great addition to the plugin!

Contributor

fnicollet commented Feb 6, 2015

Great work @manleyjster , this is a great addition to the plugin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment