-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add methods for inspecting GeoJSON clusters #6829
Conversation
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.
I think it's fine with manual test page and without vt-pbf packing for now, but the flow types could be more precise -- see inline comments.
src/source/geojson_source.js
Outdated
* @param callback A callback to be called when the zoom value is retrieved (`(error, zoom) => { ... }`). | ||
* @returns {GeoJSONSource} this | ||
*/ | ||
getClusterExpansionZoom(clusterId: number, callback: Function) { |
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.
Type the callbacks more precisely, e.g. Callback<number>
.
src/source/geojson_source.js
Outdated
* @param callback A callback to be called when the features are retrieved (`(error, features) => { ... }`). | ||
* @returns {GeoJSONSource} this | ||
*/ | ||
getClusterChildren(clusterId: number, callback: Function) { |
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.
Callback<Array<GeoJSONFeature>>
src/source/geojson_worker_source.js
Outdated
|
||
// supercluster methods | ||
getClusterExpansionZoom(clusterId: number): number; | ||
getChildren(clusterId: number): GeoJSON[]; |
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.
We're using the Array<GeoJSON>
convention rather than GeoJSON[]
.
src/source/geojson_worker_source.js
Outdated
@@ -37,6 +37,12 @@ export type LoadGeoJSONParameters = { | |||
export type LoadGeoJSON = (params: LoadGeoJSONParameters, callback: Callback<mixed>) => void; | |||
|
|||
export interface GeoJSONIndex { | |||
getTile(z: number, x: number, y: number): GeoJSON; |
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.
Let's replace export type GeoJSON = Object
above with types from flow-typed/geojson.js
.
Is there any good way to find the cluster_id associated with a given piece of data? I have a map with clusters on it. When people search for something and select it, I want to highlight which cluster it's a part of. |
@lewis500 I guess you could use |
Closes #3318. Introduces 3 new methods to clustered GeoJSON sources:
getChildren
for returning cluster's immediate children (from the next zoom)getLeaves
for returning original points that belong to the given cluster, with pagination support.getClusterExpansionZoom
for returning the zoom the cluster expands on.Misc. open questions:
GeoJSONSource#setData
, but they don't actually test the roundtrip — they just mock the dispatchersend
method, so for this case I doubt such tests will be useful.geojson_source_worker
enough, or should we try doing something elaborate (e.g. setting up Supercluster/GeoJSON-VT typings, casting the index to either etc.)? CurrentlyGeoJSONIndex
interface in the master branch is empty and typing checks for it don't run at all for some reason — e.g. there'sthis._geoJSONIndex.getTile(...)
, this method isn't typed anywhere but Flow doesn't complain.Launch Checklist
post benchmark scores