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

Add date range to overpass query #39

Merged
merged 2 commits into from
Dec 13, 2018

Conversation

kellyi
Copy link
Contributor

@kellyi kellyi commented Dec 12, 2018

Overview

Although the UI had a date range dropdown, making a selection there previously did not change the overpass query. This PR adjusts things such that selecting one of the date range options will change the query using the newer option:

https://wiki.openstreetmap.org/wiki/Overpass_API/Overpass_QL#newer

I brought in the momentjs library to handle structuring the dates more easily and ensure they would work on different browsers. Unfortunately, however, its toISOString method did not output exactly the format expected by Overpass -- so the code here manually formats it.

Connects #38

Notes

First commit here fixes a minor style issue whereby the geocoder coordinate inputs would overextend their horizontal bounds. Glance at the coordinate inputs to verify that they no longer do.

Testing Instructions

  • get this branch, then ./scripts/update
  • visit the app, select a bbox, then select "Amenties" and click "Extract"
  • verify that data returns
  • with the same bbox, select "Amenities" and try out each of the date range options in the dropdown, verifying that each of them returns results and that the visual set of results differs slightly for each date range period.

@kellyi
Copy link
Contributor Author

kellyi commented Dec 12, 2018

Hold off on reviewing this for now. I noticed two things I need to fix.

@kellyi kellyi force-pushed the ki/add-date-range-to-overpass-query branch from e136420 to 077f249 Compare December 12, 2018 21:49
@kellyi kellyi force-pushed the ki/add-date-range-to-overpass-query branch from 077f249 to f341f70 Compare December 12, 2018 22:40
@kellyi
Copy link
Contributor Author

kellyi commented Dec 12, 2018

Ok! This is ready for review. It turns out I had a stray ( in one of the functions to create a query with a newer:date addition. Extraction should now work for all queries and date combinations.

I did notice that there's not much good feedback for the user when a query completes successfully but does not return any features -- such as looking for Emergency Infrastructure in the last month. That request will 200 but since there are no features, no shapefile gets generated. I'm going to make an enhancement card for adding a visual indication that "No features were found".

@kellyi
Copy link
Contributor Author

kellyi commented Dec 12, 2018

#40 is for handling the "No features found" indication

Copy link
Contributor

@mmcfarland mmcfarland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works and looks good. Got a little tripped up on the construction of the functions for generating the query, but seems to be a thing that makes sense after you spend some time parsing it. I'm satisfied with it if it's reasonably conventional.

relation["${tag}"]${bbox}(newer:"${dateSelection}");
`;

const createOverpassDateValueQueryElement = (bbox, dateSelection) => (tag, value) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this common syntax for functions that return functions? Took me a bit of scrolling up and down to put in all together wrt to declarations and usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's the ES6 way to do currying or partial application. The lodash version of this would read a little more nicely but this project doesn't have that library and it didn't seem worthwhile to bring it in just for this.

I suppose these could be named more clearly to indicate that the initial functions are partially applied, but we can revisit if it becomes necessary.

@mmcfarland mmcfarland assigned kellyi and unassigned mmcfarland Dec 13, 2018
@kellyi
Copy link
Contributor Author

kellyi commented Dec 13, 2018

Thanks for your help with this!

@kellyi kellyi merged commit 7c3019c into develop Dec 13, 2018
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.

3 participants