Add optional custom geocoder callback function #6915
Conversation
|
Thanks for the pull request @hpinkos!
Reviewers, don't forget to make sure that:
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
Source/Widgets/Geocoder/Geocoder.js
Outdated
| @@ -34,6 +34,7 @@ define([ | |||
| * @param {GeocoderService[]} [options.geocoderServices] The geocoder services to be used | |||
| * @param {Boolean} [options.autoComplete = true] True if the geocoder should query as the user types to autocomplete | |||
| * @param {Number} [options.flightDuration=1.5] The duration of the camera flight to an entered location, in seconds. | |||
| * @param {Geocoder~DestinationFoundFunction} [options.destinationFound] A callback function that is called after a successful geocode. If not supplied, the default behavior is to fly the camera to the result destination. | |||
There was a problem hiding this comment.
I'm confused about the use case here. If a user needs to geocode and then do something with the results, they should just be calling the underlying Geocoder service directly. Why would they be using the Geocoder widget if they didn't want to fly to the destination (which is the main point of the widget)?
There was a problem hiding this comment.
It's useful to be able to use the UI without necessarily imposing the flyto behavior. For example, say I wanted to use the search box to add billboards at certain addresses on the globe.
There was a problem hiding this comment.
Perhaps expose GeocoderViewModel.flyToDestination as a documented function and assign it as the default value here so that it's more obvious that changing it to something else will remove that functionality? I think that would work better than trying to explain it in that last sentence.
There was a problem hiding this comment.
Also, does it make sense for this to be a Command? It's not called through the knockout bindings right? If that's the case a simple function callback makes more sense.
There was a problem hiding this comment.
Should it be passed in as a Command? I use that callback function to create a command: https://github.com/AnalyticalGraphicsInc/cesium/blob/386023433ddb1879cef4ef1eb89e0a7759d4be4b/Source/Widgets/Geocoder/GeocoderViewModel.js#L175-L179
There was a problem hiding this comment.
Sorry, what I meant was does it make sense for this.destinationFoundCommand to be a command at all? Why not just have it stay a callback function?
There was a problem hiding this comment.
Oh gotcha. Yeah, I'll change it
|
Update CHANGES |
|
I'll look at this in full later today. |
|
@mramato updated CHANGES |
|
I think those are the only other commends I have. Thanks @hpinkos |
hpinkos
changed the title
|
@mramato ready for another look |
|
Thanks again. |
Adds the ability to pass a custom callback function to the
Geocoderto make it have custom behavior on a successful geocode. If one isn't provided, the default is still to fly to the destination.Also cleaned up the use of spy functions in the specs.