Leaflet / Leaflet.Editable

Make geometries editable in Leaflet.
http://leaflet.github.io/Leaflet.Editable/doc/api.html
559 stars 197 forks source link

Leaflet 1.1.0 compat #130

Closed yohanboniface closed 7 years ago

yohanboniface commented 7 years ago
yohanboniface commented 7 years ago

Current status:

$ npm test

> leaflet-editable@1.0.0 test /home/ybon/Code/js/Leaflet.Editable
> make test

  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․PhantomJS has crashed. Please read the bug reporting guide at
<http://phantomjs.org/bug-reporting.html> and file a bug report.

Ilovejavascript

theashyster commented 7 years ago

Hi @yohanboniface I wanted to do this, because we rely on Leaflet and Leaflet.Editable and I would love to update the dependencies, but got stuck on the 2nd point. Kind of don't know what to do in this situation.

In L.Editable.MiddleMarker.onMouseDown I was able to replace L.Draggable._dragging = false with

var draggable = new L.Draggable(parent);
draggable.finishDrag();

and it works and the tests are also passing, but I am not convinced that this is the correct way of doing it.

In L.Editable.BaseEditor.cancelDrawing I just removed it and I also think it is probably not correct to do it like this.

yohanboniface commented 7 years ago

hi @theashyster, thanks for look at this, and sorry for the lag! The issue here as you may have understood is that L.Draggable._dragging is now private, so we cannot change it anymore. It's a bit tricky because we need to transfer the dragging from one element to another. Your suggestion may work, but we'd ideally have a more concise hack, to prevent too much side effects. I'll try to deal with this on Leaflet side.

theashyster commented 7 years ago

Hi @yohanboniface, thanks for the reply. Yep, I noticed that and understand that it would be quite hard to achieve the transfer of dragging between elements with the new code base of Leaflet 1.1.0. My suggestion worked in some fashion, but I already saw a room for errors due to the side effects that could happen, so I don't like my suggestion myself. Okay, I would be really thankful for this to be sorted out, so good luck in finding the correct fix. 👍

yohanboniface commented 7 years ago

@theashyster can you test leaflet1.1 branch with latest Leaflet master?

theashyster commented 7 years ago

Hi, @yohanboniface

I used the leaflet1.1 branch from Editable and tested it out with the latest Leaflet master in the application I am working with and all went smooth and works just fine. Of course, there are many console warnings now because of this https://github.com/Leaflet/Leaflet/commit/b675753422d7828ea379a9a0da68b62c6cdc7f76#diff-4621c57b392f8ea467790bc0f8eaecf3R240, but I guess you will update the pull request to use the isFlat function call in Editable as well here https://github.com/Leaflet/Leaflet.Editable/pull/139

Otherwise everything seems to be alright 👍

theashyster commented 7 years ago

@yohanboniface I think this issue is safe to close.