-
Notifications
You must be signed in to change notification settings - Fork 994
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
Add the ability to draw curves #1008
base: develop
Are you sure you want to change the base?
Conversation
It would be really nice if someone spend some time on reviewing this issue - I spent quite some time on it and I think it is a nice feature to add to the project. Thank you ! |
It is pretty clear that this plugin is abandoned, see https://github.com/Leaflet/Leaflet.draw/graphs/contributors Not sure is there a viable replacement - alternative project or a maintained fork |
@johnd0e here |
I do not like this approach, when 3rd-party dependencies need to have special support from main library. |
'draw/handler/Draw.Marker.js', | ||
'draw/handler/Draw.CircleMarker.js', | ||
'draw/handler/Draw.Circle.js' | ||
'draw/handler/Draw.Marker.js', | ||
'draw/handler/Draw.CircleMarker.js', | ||
'draw/handler/Draw.Circle.js' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whitespace-only changes should be avoided
build/deps.js
Outdated
@@ -2,37 +2,44 @@ var deps = { | |||
Core: { | |||
src: [ | |||
'Leaflet.draw.js', | |||
'Leaflet.Draw.Event.js' | |||
'Leaflet.Draw.Event.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meaningless change for this PR
'edit/handler/Edit.SimpleShape.js', | ||
'edit/handler/Edit.Rectangle.js', | ||
'edit/handler/Edit.CircleMarker.js', | ||
'edit/handler/Edit.CircleMarker.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
docs/examples-0.7.x/full.html
Outdated
@@ -43,6 +46,8 @@ | |||
<script src="../../src/edit/handler/Edit.Marker.js"></script> | |||
<script src="../../src/edit/handler/Edit.CircleMarker.js"></script> | |||
<script src="../../src/edit/handler/Edit.Circle.js"></script> | |||
<script src="../../src/edit/handler/Edit.Curve.js"></script> | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whitespace?
docs/examples/full.html
Outdated
@@ -43,6 +46,8 @@ | |||
<script src="../../src/edit/handler/Edit.Marker.js"></script> | |||
<script src="../../src/edit/handler/Edit.CircleMarker.js"></script> | |||
<script src="../../src/edit/handler/Edit.Circle.js"></script> | |||
<script src="../../src/edit/handler/Edit.Curve.js"></script> | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace?
src/edit/handler/EditToolbar.Edit.js
Outdated
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meaningless
src/edit/handler/EditToolbar.Edit.js
Outdated
@@ -73,7 +73,6 @@ L.EditToolbar.Edit = L.Handler.extend({ | |||
|
|||
if (map) { | |||
map.getContainer().focus(); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meaningless
@@ -191,7 +196,6 @@ L.EditToolbar.Edit = L.Handler.extend({ | |||
_enableLayerEdit: function (e) { | |||
var layer = e.layer || e.target || e, | |||
pathOptions, poly; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meaningless
src/edit/handler/EditToolbar.Edit.js
Outdated
@@ -215,6 +219,7 @@ L.EditToolbar.Edit = L.Handler.extend({ | |||
|
|||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meaningless
src/leaflet.draw.css
Outdated
@@ -152,6 +152,10 @@ | |||
.leaflet-draw-toolbar .leaflet-draw-draw-polyline { | |||
background-position: -2px -2px; | |||
} | |||
.leaflet-draw-toolbar .leaflet-draw-draw-curve { | |||
background-image: url('images/bezier.png') !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!important
should be avoided
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will integrate the new icon into the spritesheet.
@johnd0e About your comment, I agree that it is not ideal. I guess a solution would be to create yet an other repo making the bridge between Leaflet.Draw and Leaflet.Curve, but that would require some changes in the architecture of Leaflet.Draw that are beyond the scope of this PR. |
It seems that the travis checks are stuck, I tried closing and reopening the issue, with no success. |
Check out leaflet-geoman. It practically does everything leaflet-draw does and then some. Plus it's not, you know, abandoned. |
Adding the optional dependency Leaflet.curve will enable the drawing of curves.
It is only available for Leaflet >= 1.0. and with the L.curve library imported, with a graceful fallback if it's not the case.
I would appreciate a review and get your comments if anything is missing.