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

Optimize bbox queries #129

Open
jwilson8767 opened this issue Oct 16, 2020 · 5 comments
Open

Optimize bbox queries #129

jwilson8767 opened this issue Oct 16, 2020 · 5 comments

Comments

@jwilson8767
Copy link
Contributor

In stationsMap.js we load a JSON file of points (lat/long) into ArcGIS JS API, then (later) convert them to geoJSON and then use Turf.js to do point-in-polygon math. We can improve this by using the built-in functionality of the ArcGIS JS API (queryFeatures) or even just use simple math operations. This will save loading Turf.js(128kb package), and save copying the original points several times.

@daveism
Copy link
Member

daveism commented Oct 16, 2020

Actually, I disagree. I find turf to be more performant than the ArcGIS JS API, and Turf is logically easier to follow. Maybe we can set up a simple test and see.

@jwilson8767
Copy link
Contributor Author

I'm not saying Turf is slower than the JS API. I'm saying in this case, the way we're loading both is unnecessary and that this particular problem does not require the 128kb package that is Turf. In fact, this problem can be solved using simple math because we're only checking if the points are within a given bbox.

@jwilson8767
Copy link
Contributor Author

I also don't think this is urgent, was just bookmarking for the next time we're working on the stationsMap.js or are looking to trim some dependencies.

@daveism
Copy link
Member

daveism commented Oct 16, 2020

actually we are checking if the points are in the box of the map extent. We do this to avoid presenting the entire list of stations when the user is viewing a county. During usability testing particpants did not exepect the entire list, they expected the list to only contain the stations within the map extent.

@jwilson8767
Copy link
Contributor Author

That still doesn't require a geospatial library.

@cmy2k cmy2k added this to In progress in 2021 CE improvements Jan 11, 2021
@cmy2k cmy2k removed this from In progress in 2021 CE improvements Jan 11, 2021
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

No branches or pull requests

2 participants