-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: clickFeatures
mode
#48
Conversation
✔️ Deploy Preview for oslmap ready! 🔨 Explore the source changes: 61f41b4 🔍 Inspect the deploy log: https://app.netlify.com/sites/oslmap/deploys/61288ca7b580820007ebef68 😎 Browse the preview: https://deploy-preview-48--oslmap.netlify.app |
@@ -72,6 +71,9 @@ export class MyMap extends LitElement { | |||
@property({ type: Boolean }) | |||
showFeaturesAtPoint = false; | |||
|
|||
@property({ type: Boolean }) | |||
clickFeatures = false; |
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.
Been meaning to review this PR properly but in the meantime just wanted to mention this. These boolean arguments make me worry about hitting a boolean trap. It doesn't seem to be the case quite yet, as there seems to be no intersection between the existing arguments, but I can see how we might see the existing code, and follow existing pattern, and mindlessly add more boolean arguments that do create traps. Just thinking out loud.
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 for looking out! I agree this is a tricky balance - for now, I am trying to minimally/thoughtfully introduce new boolean properties, and sticking to a rule of thumb that they all be false
by default until a user enables them.
Something I see often in other map libraries is the concept of "modes", where mode could be a string property and we'd enable/specify one mode at a time - like "draw", "showFeature", "clickFeatures", "showGeojson" etc. But the tricky nature of this project is our requirement to mix & match between these various modes & allow the functionalities to co-exist -- hence how I settled on the booleans in the first place.
But definitley a good point to chew on as we think about how to best stabilize our API going forward!
Merging this one to make it available in the next minor release today. It's a feature addition, disabled by default, and re-implements 'union' logic from our early prototype, so I don't expect it to introduce any breaks. Please feel free to share any more feedback post-merge though! |
feels like this is popping up as a natural next step in re-imagining the FindProperty to DrawBoundary transition in planx - the idea here is that we've highlighted the building footprint (or whatever single feature) that intersects with the address point, so now we can turn on ability to click/merge the yard, etc and achieve a 'site boundary' without drawing
changes:
clickFeatures
boolean property, which is only supported whenshowFeaturesAtPoint
is also enabled for nowfitToData
Closes #47