Skip to content
This repository has been archived by the owner on Feb 2, 2023. It is now read-only.

feat(controlled-search): expose search method in places.js #659

Merged
merged 3 commits into from
Feb 25, 2019

Conversation

JonathanMontane
Copy link
Contributor

Summary

Before this PR, in order to do a query programmatically, you had to use the JS api client with the .initPlaces method. This was problematic for multiple reasons, the first one being that this increased bundle size, and also meant learning about the response from the API which is a bit more complex than the pre-formatted suggestion objects that are exposed in the places.js library, and combining the two different format is error-prone.

With this PR, the search method is now accessible at the instance level.

Result

The shape of the search method is:

placesInstance.search(query: String): Promise<Array<Suggestion>>

Important: the exposed search method uses the internal configuration of the places instance to apply its filter. The reason for that is that this .search method is simply a wrapper around the search function generated by the createDataSource function, and this search function only accepts a query and a callback as parameters, and does not accept additional options to use for that query. Modifying that behaviour would imply a change in the interface which would be a breaking change, so I did not do that in that PR.

If we think that this breaking change is worth it, we can work on it while preparing a v2.0.0 of Places.

Updated Example with .search method:
image
image

@raphi
Copy link
Contributor

raphi commented Jan 25, 2019

@JonathanMontane should we move forward with this one?

@JonathanMontane
Copy link
Contributor Author

It was low priority, but it can be reviewed.
I'll just add a test and fix the conflict.

@MichalZalecki
Copy link

@JonathanMontane What's the status on this PR? Would be great to have it merged! 👏

Copy link
Contributor

@raphi raphi left a comment

Choose a reason for hiding this comment

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

LGTM

@JonathanMontane JonathanMontane merged commit 4ddd681 into master Feb 25, 2019
@raphi
Copy link
Contributor

raphi commented Feb 25, 2019

@MichalZalecki sorry for the delay, incoming 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants