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

feat: clickFeatures mode #48

Merged
merged 3 commits into from
Aug 27, 2021
Merged

feat: clickFeatures mode #48

merged 3 commits into from
Aug 27, 2021

Conversation

jessicamcinchak
Copy link
Member

@jessicamcinchak jessicamcinchak commented Aug 19, 2021

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

chrome-capture

changes:

  • add clickFeatures boolean property, which is only supported when showFeaturesAtPoint is also enabled for now
  • outlineSource/outlineLayer renamed to geojsonSource/geojsonLayer; outline naming now used to reference merged features
  • query features on single click and merge into outlineSource, calculate combined area
  • add utility function fitToData
  • update README

Closes #47

@netlify
Copy link

netlify bot commented Aug 19, 2021

✔️ 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

@jessicamcinchak jessicamcinchak marked this pull request as ready for review August 24, 2021 14:50
@jessicamcinchak jessicamcinchak requested a review from a team August 24, 2021 16:11
@@ -72,6 +71,9 @@ export class MyMap extends LitElement {
@property({ type: Boolean })
showFeaturesAtPoint = false;

@property({ type: Boolean })
clickFeatures = false;
Copy link
Contributor

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.

Copy link
Member Author

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!

@jessicamcinchak
Copy link
Member Author

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!

@jessicamcinchak jessicamcinchak merged commit 07ab0cd into main Aug 27, 2021
@jessicamcinchak jessicamcinchak deleted the jess/click-features branch August 27, 2021 07:14
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.

Reimplement click to select & combine features
2 participants