-
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: add support for drawing many features on the same map #466
Conversation
✅ Deploy Preview for oslmap ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
sketch.set("area", formatArea(sketchGeom, this.areaUnit)); | ||
} | ||
}); | ||
} |
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.
This is breaking change areaChange
event when drawing polygons, this proposes simply setting area
within the geojson properties
and dispatching the single geojsonChange
event.
A couple benefits:
- Scales much more nicely between draw-one and draw-many, naturally accounts for extra
label
property too - End consumers only have to subscribe to a single thing and just parse different data from it
- I can't think of any current use cases where you'd only subscribe to an
areaChange
and not care about the wholegeojsonChange
??
Any objections or alternative ideas?
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.
Yeah this is super interesting. I think that this approach will work well and suit our requirements - I can't really think of an issue here.
Right now we just need the general concept of a change event, and don't really need granular details (e.g. to inform a consumer that the area property of feature 14 have updated). It's enough to say there's been an update to the FeatureCollection as a whole, and it will need to be updated. It's possible that in the future this could be required (seems unlikely and parsable by consumers anyway?), but for now a single "change" event is a lot simpler and meets all requirements 👍
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.
Nice change with a very small diff - love to see it 🙌
sketch.set("area", formatArea(sketchGeom, this.areaUnit)); | ||
} | ||
}); | ||
} |
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.
Yeah this is super interesting. I think that this approach will work well and suit our requirements - I can't really think of an issue here.
Right now we just need the general concept of a change event, and don't really need granular details (e.g. to inform a consumer that the area property of feature 14 have updated). It's enough to say there's been an update to the FeatureCollection as a whole, and it will need to be updated. It's possible that in the future this could be required (seems unlikely and parsable by consumers anyway?), but for now a single "change" event is a lot simpler and meets all requirements 👍
Changes:
drawMany
prop which has default valuefalse
drawType
"Polygon" or "Point"drawMany
is true (label text is auto-index)geojsonChange
event to setproperties
label
will match visual text on map for anydrawType
area
replaces previously separateareaChange
event whendrawType="Polygon"
- see commentindex.ts
- sorry for extra noise!Future scope / follow-up PRs: