Refactor search providers #7682
Conversation
| get autocompleteEnabled() { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Why override this here when it is already part of LocationSearchProviderTraits and is defaulting to true? Same for the other providers.
There was a problem hiding this comment.
I just wanted to be explicit for all search providers, I can remove it if necessary
There was a problem hiding this comment.
Actually I was running into a bug that the value wasn't propagated correctly
|
|
||
| supportsAutocomplete(): boolean { | ||
| return true; | ||
| async search(searchText: string, manuallyTriggered?: boolean) { |
There was a problem hiding this comment.
Doubt: Could you explain when manuallyTriggered is true and why we need to differentiate both?
There was a problem hiding this comment.
manuallyTriggered is when the user presses enter to start the search; in that case, we need to cancel the debounce and run the search without waiting
| searchState.catalogSearchResults?.results | ||
| ? searchState.catalogSearchResults.results.map( | ||
| searchState.catalogSearchProvider?.result.results | ||
| ? searchState.catalogSearchProvider.result.results.map( |
There was a problem hiding this comment.
This should be catalogSearchProvider.searchResult instead (currently this results in app crash when searching from catalog explorer modal)
| const escapedKeyword = keywordToHighlight.replace( | ||
| /[.*+?^${}()|[\]\\]/g, | ||
| "\\$&" | ||
| ); |
There was a problem hiding this comment.
BTW, there is a new RegExp.escape, although it is pretty new.
There was a problem hiding this comment.
I would avoid moving it for some time, as it is still pretty new although it would work for most of the users, and this is enough to resolve the issue we are currently seeing
|
@zoran995 - Looks good, just a few comments. |
na9da
left a comment
na9da
left a comment
There was a problem hiding this comment.
Search from mobile view must also be fixed.
zoran995
force-pushed
the
search-refactor
branch
from
fbeaa8b to
10adef0
Compare
zoran995
force-pushed
the
search-refactor
branch
from
3b15483 to
2852ad1
Compare
What this PR does
Fixes #
During testing the search providers, I discovered that search debounce doesn't work correctly and that we can achieve a better UX when autocomplete is disabled for some search providers.
Before:
After:
Test me
How should reviewers test this? (Hint: If you want to provide a test catalog item, create a Gist of its catalog JSON, add its url and your branch name to this url:
http://ci.terria.io/<branch name>/#clean&<raw url of your gist>, and then paste that in the body of this PR)Checklist
doc/.