-
Notifications
You must be signed in to change notification settings - Fork 234
feat(is-routing): support instantsearch routing #551
Conversation
makes Places InstantSearch Widget support the routing API from InstantSearch.
src/instantsearch/widget.test.js
Outdated
const getInitializedWidget = () => { | ||
const client = createFakeClient(); | ||
const helper = createFakekHelper(client); | ||
const widget = algoliaPlacesWidget({ defaultPosition: [2, 2] }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there probably should be a test without defaultPosition
too
@@ -10,34 +10,68 @@ class AlgoliaPlacesWidget { | |||
} | |||
|
|||
this.placesOptions = placesOptions; | |||
this.placesAutocomplete = places(this.placesOptions); | |||
|
|||
this.state = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not completely sure, but this could be called this.uiState
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't leave any more comments and it's working, so LGTM!
src/instantsearch/widget.js
Outdated
(uiState.places.position === this.state.position && | ||
uiState.places.query === this.state.query) | ||
) { | ||
this.placesAutocomplete.setVal(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition is when uistate.places is not define or position and query are the same which doesn't mean that the query is empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good and congratulations on the implementation of the instantsearch routing for this widget @JonathanMontane :)
.setQueryParameter('aroundLatLng', this.defaultPosition); | ||
.setQueryParameter( | ||
'aroundLatLng', | ||
this.state.position || this.defaultPosition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this.state.position
will ever be defined in that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be defined because of the init of the routing which happens before the init of the widget.
}, | ||
} = opts; | ||
|
||
this.state.position = `${lat},${lng}`; | ||
this.state.query = value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than duplicate the source of truth for the values why doesn't read them directly from the helper & placesAutocomplete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the query you're looking for. It's actually the value for searching for values in places, which is nowhere in the main search state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but since you have access to the Places instance you can have access to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although you can simplify the internal state by removing the position information, you cannot make it completely disappear, as the getVal
method from the placesAutocomplete instance will yield stale data.
the placesAutocomplete.on('change', () => {})
is triggered before the update of the value of the input, and so is getWidgetState
. So placesAutocomplete.getVal
will yield the previous input value, instead of the new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thanks for the update!
Summary
This PR makes Places InstantSearch Widget support the routing API from InstantSearch.
Result
The Places widget now exposes the following object to the
uiState
:position
is the variable used to update thearoundLatLng
query
is the value of the input when a places record has been selectedDemo
https://places-routing.surge.sh