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

Add the ability to draw curves #1008

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

Add the ability to draw curves #1008

wants to merge 11 commits into from

Conversation

qpincon
Copy link

@qpincon qpincon commented Mar 15, 2021

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.

@qpincon
Copy link
Author

qpincon commented Mar 22, 2021

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 !

@matkoniecz
Copy link

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

@t1a2l
Copy link

t1a2l commented Apr 10, 2021

@johnd0e‬‏ here

@johnd0e
Copy link

johnd0e commented Apr 10, 2021

Adding the optional dependency

I do not like this approach, when 3rd-party dependencies need to have special support from main library.
It would be better to have some extension methods to be able to add any such feature.

Comment on lines -17 to +25
'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'
Copy link

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',
Copy link

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',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@@ -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>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace?

@@ -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>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace?


Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meaningless

@@ -73,7 +73,6 @@ L.EditToolbar.Edit = L.Handler.extend({

if (map) {
map.getContainer().focus();

Copy link

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;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meaningless

@@ -215,6 +219,7 @@ L.EditToolbar.Edit = L.Handler.extend({

}


Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meaningless

@@ -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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!important should be avoided

Copy link
Author

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.

@qpincon
Copy link
Author

qpincon commented Jul 28, 2021

@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.
I suggest merging this PR as-is (if all is good), and migrate in the future if this new architecture comes to life.

@qpincon qpincon closed this Aug 6, 2021
@qpincon qpincon reopened this Aug 6, 2021
@qpincon
Copy link
Author

qpincon commented Aug 6, 2021

It seems that the travis checks are stuck, I tried closing and reopening the issue, with no success.

@patrickoliveras
Copy link

patrickoliveras commented Aug 31, 2021

Check out leaflet-geoman. It practically does everything leaflet-draw does and then some. Plus it's not, you know, abandoned.

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.

None yet

6 participants