Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable marker editing #323

Merged
merged 9 commits into from
Oct 6, 2014
Merged

Conversation

justinmanley
Copy link
Member

L.EditToolbar.Edit decides how to handle layers based on the layer type. The process goes like this:

  • If layer instanceof L.Marker is true, then L.EditToolbar.Edit does a series of marker-specific editing actions.
  • Otherwise, L.EditToolbar.Edit does a few style tweaks, then calls layer.editing.enable(), delegating the specific editing behavior to the handler for that layer (i.e. L.Edit.Rectangle, L.Edit.Circle, L.Edit.Polyline, etc.). This means that layer.editing.enable() is not called if layer inherits from L.Marker.

This was a problem for me because I have a complex layer, L.Illustrate.Textbox, which inherits from L.Marker, but needs to have custom editing behavior.

As I looked at the code for L.EditToolbar.Edit, it seemed that the differential handling of layers into based on marker / non-marker type was fairly arbitrary. Furthermore, there were two lengthy marker-specific methods - _toggleMarkerHighlight and _offsetMarker - in L.EditToolbar.Edit that didn't seem to belong to a general-purpose editing class.

I have refactored L.EditToolbar.Edit to address these issues, moving marker-specific behavior into a new L.Edit.Marker class. This is nice because it makes layer.editing.enable() a universal method for turning on editing behavior, regardless of the layer type. As a bonus, it also makes it easy for plugin developers to override the default editing behavior for classes extending L.Marker.

@justinmanley
Copy link
Member Author

Just added two new tests in spec/EditSpec.js for L.Edit.Marker and L.Edit.Circle to make sure that changes don't break anything.

@justinmanley
Copy link
Member Author

@jacobtoye @danzel any thoughts on this?

@danzel
Copy link
Member

danzel commented Oct 3, 2014

Please remove your changes from the dist/ folder (leaflet.draw-src.js and leaflet.draw.js) These get rebuilt after a merge. They are probably what is causing the merge error.

I don't know if it is possible, but could we remove the remaining type checking too? (the if (!isMarker) { bits) by using the same magic?

@jacobtoye
Copy link
Member

Awesome pull @manleyjster! Sorry I haven't gotten onto this earlier. Like @danzel says could you please back out the changes to the /dist folder. Once you've done that I'll merge the changes.

@justinmanley
Copy link
Member Author

I've removed changes from dist/ (79161ea) but want to look into the remaining isMarker conditionals before the PR goes through, as @danzel suggested. I'll post again when I've looked into that.

…rs: Remove 'isMarker' tests from L.EditToolbar.Edit and set layer styles for editing mode in L.Edit.Poly and L.Edit.SimpleShape, rather than L.EditToolbar.Edit.
@justinmanley
Copy link
Member Author

I've removed the remaining type checking for isMarker as @danzel suggested.

This involved tweaking how styles for editing mode are set. The options object is still composed in L.EditToolbar.Edit, but since L.Marker does not have a setStyle method, I've moved the call to layer.setStyle out of L.EditToolbar.Edit and into L.Edit.Poly and L.Edit.SimpleShape.

I've added two more tests to spec/EditSpec.js to make sure that the styles are changed and reverted correctly during editing mode.

PR should be ready for merge.

jacobtoye added a commit that referenced this pull request Oct 6, 2014
@jacobtoye jacobtoye merged commit ccca4b1 into Leaflet:master Oct 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants