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

Leaflet 1.1.0 compat #130

Closed
2 tasks
yohanboniface opened this issue Jun 29, 2017 · 7 comments
Closed
2 tasks

Leaflet 1.1.0 compat #130

yohanboniface opened this issue Jun 29, 2017 · 7 comments

Comments

@yohanboniface
Copy link
Member

yohanboniface commented Jun 29, 2017

  • L.Polyline._flat replaced by L.LineUtil._flat
  • L.Draggable._dragging is now private, preventing the middlemarker hack
@yohanboniface
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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 added a commit to Leaflet/Leaflet that referenced this issue Jul 24, 2017
yohanboniface added a commit to Leaflet/Leaflet that referenced this issue Jul 24, 2017
mourner pushed a commit to Leaflet/Leaflet that referenced this issue Jul 24, 2017
@yohanboniface
Copy link
Member Author

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

@theashyster
Copy link
Contributor

theashyster commented Jul 31, 2017

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 Leaflet/Leaflet@b675753#diff-4621c57b392f8ea467790bc0f8eaecf3R240, but I guess you will update the pull request to use the isFlat function call in Editable as well here #139

Otherwise everything seems to be alright 👍

@theashyster
Copy link
Contributor

@yohanboniface I think this issue is safe to close.

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

No branches or pull requests

2 participants