Skip to content

feat: clickFeatures mode #48

Merged
jessicamcinchak merged 3 commits intomainfrom
jess/click-features
Aug 27, 2021
Merged

feat: clickFeatures mode #48
jessicamcinchak merged 3 commits intomainfrom
jess/click-features

Conversation

@jessicamcinchak
Copy link
Copy Markdown
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
Copy Markdown

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
Comment thread src/my-map.ts
showFeaturesAtPoint = false;

@property({ type: Boolean })
clickFeatures = false;
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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