feat(searchResultsByProperty): No results found message when no results for any property #2159
Conversation
Coverage of commit
|
Coverage of commit
|
KaylaBrady
force-pushed
the
kb-no-search-results-msg
branch
from
9f3ddbf
to
739ba8c
Compare
Coverage of commit
|
…for all properties
KaylaBrady
force-pushed
the
kb-no-search-results-msg
branch
from
6023cc4
to
65cce65
Compare
Coverage of commit
|
| const useSearchResultsByProperty = ( | ||
| socket: Socket | undefined, | ||
| text: string, | ||
| properties: PropertyLimits | ||
| ): SearchResultsByProperty => { |
There was a problem hiding this comment.
question: do you think it'd be useful to rewrite useAutocompleteResults using this function? Seems like we ended up doing the same thing heh!
There was a problem hiding this comment.
Oh shoot, sorry I didn't realize we were overlapping there! My preference would probably be that we keep these independent for now to prevent blocking each other with merges, but maybe do a cleanup ticket to consolidate if it ends up seeming useful?
There was a problem hiding this comment.
No worries!
and a cleanup ticket afterward sounds good to me 👍🏻
There was a problem hiding this comment.
One other thing to consider is that the functionality for location autocomplete vs. results will actually be different (though that can probably be handled by an extra argument to the hook specifying which one you want).
| )} | ||
| </section> | ||
| ) | ||
| } | ||
|
|
||
| const ShowMore = ({ property }: { property: SearchProperty }) => { |
There was a problem hiding this comment.
praise: thanks for pulling this out into a separate component, I should have done this originally in my PR.
| export type SearchResultsByProperty = { | ||
| vehicle: VehicleResultType | ||
| operator: VehicleResultType | ||
| run: VehicleResultType | ||
| location: LocationResultsType | ||
| } |
There was a problem hiding this comment.
question(non-blocking): Would making this generic make it easier to share this code between autocomplete and search results without making the code too complex?
maybe something along the lines of this?
| export type SearchResultsByProperty = { | |
| vehicle: VehicleResultType | |
| operator: VehicleResultType | |
| run: VehicleResultType | |
| location: LocationResultsType | |
| } | |
| export type SearchPropertyResults<TVehicleResult, TLocationResult> = { | |
| vehicle: TVehicleResult | |
| operator: TVehicleResult | |
| run: TVehicleResult | |
| location: TLocationResult | |
| } | |
| export type SearchResultsByProperty = SearchPropertyResults<VehicleResultType, LocationResultsType> | |
| export type AutocompleteResultsByProperty = SearchPropertyResults<VehicleResultType, LocationSuggestionsResultsType> |
There was a problem hiding this comment.
Ooh I like that idea! I wonder if it might help to make a generic SearchPropertyResult type too so they can share loading / ok / null states? I'm not sure that autocomplete will need to keep the shape of LimitedSearchResults though...
I think I'll hold off on adding generics here for now since I'm not quite seeing the full path to code sharing with autocomplete, but I support adding them as part of that work!
type SearchPropertyResult<T> = Ok<LimitedSearchResults<T>> | Loading | null
export type SearchResultsByProperty<TVehicleResult, TLocationResult> = {
vehicle: SearchPropertyResult<TVehicleResult>
operator: SearchPropertyResult<TVehicleResult>
run: SearchPropertyResult<TVehicleResult>
location: SearchPropertyResult<TLocationResult>
}
ticket: https://app.asana.com/0/1200180014510248/1204956501632025/f