Nominatim search provider #7302
Conversation
| searchResults.results.length = 0; | ||
| searchResults.message = undefined; | ||
|
|
||
| if (searchText === undefined || /^\s*$/.test(searchText)) { |
There was a problem hiding this comment.
this check is unnecessary as searchText is always defined and is longer than min length. It is validated in SearchProviderMixin
| CesiumMath.toDegrees(view.rectangle.south); | ||
|
|
||
| const promise = loadJson( | ||
| this.url + |
There was a problem hiding this comment.
might be better to have this as a Resource if possible, it will be more readable than manually constructed string
| name: "Primary country", | ||
| description: "Name of the country to prioritize the search results." | ||
| }) | ||
| countryCodes: string = "it"; |
There was a problem hiding this comment.
I believe that terria defaults should be Australia
There was a problem hiding this comment.
better to leave it undefined?
| } | ||
| } | ||
|
|
||
| function createZoomToFunction(model: NominatimSearchProvider, resource: any) { |
There was a problem hiding this comment.
Can you also add the proper type definition to the resource here instead of any? It should be Feature if I am correct.
|
also, it would be nice to add unit tests for new features p.s. Not sure what is team's standpoint whether this should reside in the terria codebase or be a plugin for terriajs, so we don't increase the codebase size and implement/maintain every possible search provider out there. It is the main reason the configurable search providers are implemented, and I didn't have time to test the possibility of using a search provider from a plugin but I don't see why it shouldn't work. @na9da what's your opinion on this |
|
@zoran995 - thanks for reviewing it. It might be possible to write this as a plugin, but will require importing several private Terria classes which is not ideal. I would accept this PR and split it out later when the use case is better supported by the plugin framework. |
|
@na9da LGTM, but I noticed one thing that might need additional consideration. Do we need to handle this somehow, as multiple requests won't play nicely with the Nominatim TOS? Users already can limit the query to specific countries using @glughi Besides the above this is in a pretty good state from the code perspective, it just needs
|
What this PR does
Added search provider for Nominatim of OpenStreetMap
Checklist
doc/.