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

Support "Near Me" queries #57

Merged
merged 21 commits into from
Nov 23, 2021
Merged

Support "Near Me" queries #57

merged 21 commits into from
Nov 23, 2021

Conversation

yen-tt
Copy link
Contributor

@yen-tt yen-tt commented Nov 17, 2021

Add support for 'Near me' queries in search bar

  • added search-operations file that includes util functions for executing search, getting search intents, and user location
  • update LocationBias component to retrieve user location and execute search using SearchHandler.
  • updated VerticalSearchPage and UniversalSearchPage fetch searchIntents from autocomplete search, in the case of a default initial search with near me intent, get and update user's location if there is a near me intent, and execute query.
  • updated searchBar to rely on autocomplete response from latest request to retrieve the search intent, and update user's location before executing a search

Note: there's a noticeable wait time when navigator.geolocation is used. and there isn't any indication that the results maybe in loading state. Should consider exposing a setLoadingStatus in answersHeadless, so it can be use when executeSearchWithUserLocation is triggered.

J=SLAP-1706
TEST=manual

  • set location to washington DC in when vertical page is loaded, click update my location from LocationBias component, and see that the label is updated to my device's location.
  • set default search query to 'engineers near me' and see that user location is set based on location near my device.
  • ran searches between non-nearMe and nearMe queries, see that setUserLocation is only on queries with nearMe intents

- added `executeSearchWithUserLocation` function

J=SLAP-1706
TEST=manual

- set location to washington DC in when vertical page is loaded, click update my location, and see that the label is updated to my device's location.
- set default search query to 'engineers near me' and see that user location is set based on location near my device.
@coveralls
Copy link

coveralls commented Nov 17, 2021

Coverage Status

Coverage remained the same at 75.532% when pulling 4f0ea01 on dev/near-me-support into ced7e2c on main.

sample-app/src/components/LocationBias.tsx Show resolved Hide resolved
sample-app/src/components/SearchBar.tsx Outdated Show resolved Hide resolved
sample-app/src/utils/geolocationutils.tsx Outdated Show resolved Hide resolved
sample-app/src/utils/geolocationutils.tsx Outdated Show resolved Hide resolved
sample-app/src/utils/geolocationutils.tsx Outdated Show resolved Hide resolved
@yen-tt yen-tt requested review from nmanu1 and cea2aj November 18, 2021 20:05
@yen-tt yen-tt requested a review from oshi97 November 19, 2021 17:57
@yen-tt yen-tt requested a review from cea2aj November 19, 2021 19:34
@yen-tt yen-tt requested a review from cea2aj November 22, 2021 17:58
@@ -17,7 +19,14 @@ export default function UniversalSearchPage(props: { universalResultsConfig: Uni
vertical: {}
})
answersActions.setVerticalKey('');
answersActions.executeUniversalQuery();
const executeQuery = async () => {
let searchIntents: SearchIntent[] = [];
Copy link
Member

@cea2aj cea2aj Nov 22, 2021

Choose a reason for hiding this comment

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

This logic looks correct, however I find it a little difficult to follow since it requires me to go back and forth between the search-operations class to understand it. Would something like this work:

if ( state.location.userLocation ) {
   executeSearch()
} else {
   const intents = getSearchIntents()
   updateLocationIfNeeded(intents)
   executeSearch()
}

Copy link
Contributor Author

@yen-tt yen-tt Nov 22, 2021

Choose a reason for hiding this comment

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

in both condition, it would still call executeSearch so I think it's better to just extract it out of the if/else and call it once. But I updated the function to be updateLocationIfNeeded as suggested, hopefully that's easier to understand

Copy link
Member

Choose a reason for hiding this comment

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

looks good!

@yen-tt yen-tt requested a review from cea2aj November 22, 2021 19:49
@yen-tt yen-tt merged commit fefc014 into main Nov 23, 2021
@yen-tt yen-tt deleted the dev/near-me-support branch November 23, 2021 15:55
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.

None yet

5 participants