-
Notifications
You must be signed in to change notification settings - Fork 361
fix: geojson layers pmIgnore #1251
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
base: master
Are you sure you want to change the base?
fix: geojson layers pmIgnore #1251
Conversation
There's probably overlap with this PR #1247 |
Hey @mangecoeur, I tested an environment with PR #1247 and Geoman was still able to modify GeoJSON layers: I think the important part of this PR that solves this issue is this: if (pmIgnore && layer.pm) {
layer.pm.disable();
delete (layer as any).pm;
} |
Thanks, actually for my current use i mashed together both my current PR and your fixes into a kind of frakenpackage because I couldn't wait for a new ipyleaflet release. Would really like to land all of these fixes in the main repo instead. See the branch here (https://github.com/mangecoeur/ipyleaflet/tree/merge_extra_geoman_prs) I was actually in such a rush that I just copy-pasted your file changes rather than merge from your branch 😅. Would rather re-do it clean instead. |
Same 😅 -- I have a custom mix and match of recent fixes. Hopefully the maintainers have some time to review these soon 👀 |
ab8a2b9
to
b17f53f
Compare
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.
Thanks @lopezvoliver
You'd just need to run jlpm run lint
in python/jupyter_leaflet
directory in order to make the CI happy
And we'll also trigger ui-tests snapshots update to hopefully make ui-tests pass
cc: @martinRenou
Works pretty nicely, the rectangle is (pm_ignore=true
) and the other polygon is not:
Screen.Recording.2025-06-04.at.17.15.24.mp4
@@ -60,6 +60,14 @@ export class LeafletGeoJSONView extends LeafletFeatureGroupView { | |||
coordinates: [e.latlng.lat, e.latlng.lng], | |||
}); | |||
}; | |||
const pmIgnore = this.model.get('pm_ignore'); |
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.
Should we handle the change:pm_ignore
event as well?
Does it mean we can safely merge both PRs without conflicts/collision? |
Similar to #1249 , this PR updates jupyter_leaflet so that individual layers created by the LeafletGeoJSONModel get the proper
pmIgnore
option -- and disable thepm
if it's already attached. This fixes the issue that Geoman is able to edit GeoJson layers, even though they inherit thepm_ignore
from the base Layer class.