Search in Android Nearby Transit #584
Conversation
JackVCurtis
force-pushed
the
jvc-android-search-in-nearby
branch
from
e3fcf9b to
1b2998d
Compare
JackVCurtis
changed the title
JackVCurtis
force-pushed
the
jvc-android-search-in-nearby
branch
from
cd1a4a1 to
6620c12
Compare
There was a problem hiding this comment.
This is a good start! A few comments about structure / functionality.
The main piece from this PR that is blocking other work is the existence of the SearchBarOverlay view. I'm OK with merging this as-is since there is a plan to add tests, and so long as these comments are addressed.
| var searchInputState by rememberSaveable { mutableStateOf("") } | ||
| val searchResults = getSearchResults(searchInputState) | ||
|
|
||
| fun handleSearch(stopId: String) { |
There was a problem hiding this comment.
suggestion(non-blocking): handleSearch seems like a misnomer here; this seems to be something along the lines of handleTapResult?
| query: String, | ||
| searchResultRepository: ISearchResultRepository = koinInject() | ||
| ): SearchResults? { | ||
| val viewModel = remember(query) { SearchResultsViewModel(searchResultRepository, query) } |
There was a problem hiding this comment.
question: Why recreate the VM on each query, vs. persist the VM & update the data it uses via a launchedEffect? I found the latter approach helpful in #588, and I think it will help with a missing piece of search functionality:
When I search for a stop, tap that stop, then click the X button to go back to nearby transit, I should see the search bar with my previous query & search results like I do on iOS.
There was a problem hiding this comment.
The difference in iOS behavior is unrelated, that was me manually clearing the state. I've switched to a persistent VM for consistency with your approach elsewhere, though.
| if ( | ||
| searchResults?.stops != null && searchResults.stops.isNotEmpty() | ||
| ) { | ||
| handleSearch(searchResults.stops[0].id) |
There was a problem hiding this comment.
question: selecting the first result by default is unexpected to me - is that an android-specific pattern?
There was a problem hiding this comment.
This is the behavior when you hit the equivalent of the "Enter" button. What do you think we should do instead?
There was a problem hiding this comment.
on the iOS-side, hitting the "done" button on search does nothing (or maybe re-runs the search in the background? but the effect is the same - you still see a list of search results to choose from, rather than automatically being taken to the first result)
JackVCurtis
force-pushed
the
jvc-android-search-in-nearby
branch
2 times, most recently
from
8a40dcb to
f4a4147
Compare
JackVCurtis
force-pushed
the
jvc-android-search-in-nearby
branch
from
560d19b to
762436f
Compare
|
Untouched iOS tests are failing due to network issues on XCode Cloud - merging anyway. |
Summary
Ticket: 🤖 | Search | Search bar in nearby transit
What is this PR for?
This adds the search bar overlay to Android and allows search by stop with navigation to stop details. UI is left for the next ticket. I've skipped testing this PR in order to unblock other work, but will be adding tests before marking the ticket complete.
iOS
android
Testing
What testing have you done?