Provide autocomplete suggestions in SearchRouterList #51237
Conversation
SzymczakJ
changed the title
|
Should we somehow limit the list of autosuggestions @luacmartins? This is POC implementation of autosuggestions for some of filters(the ones linked in description) poc.movI don't see much designs connected to autocomplete. |
|
@SzymczakJ I think we should only offer suggestions once the user typed at least one character after the filter key, e.g. |
|
Agree with Carlos that we'd suggest on the first character after the filter key. |
|
@SzymczakJ we discussed internally and the behavior we want for this is:
Does that make sense? |
| const singlePolicyTagsList: PolicyTagLists | undefined = allPoliciesTagsLists?.[`${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`]; | ||
| if (!singlePolicyTagsList) { | ||
| const uniqueTagNames = new Set<string>(); | ||
| const tagListsUnpacked = Object.values(allPoliciesTagsLists ?? {}).filter((item) => !!item) as PolicyTagLists[]; |
There was a problem hiding this comment.
is possible to use default argument instead, and do
getAutoCompleteTagsList(allPoliciesTagsLists: OnyxCollection<PolicyTagLists> = {}, policyID?: string)?
There was a problem hiding this comment.
I believe our hands are tied by OnyxCollection which can be undefined.
| const [cardList = {}] = useOnyx(ONYXKEYS.CARD_LIST); | ||
| const cardsAutocompleteList = Object.values(cardList ?? {}).map((card) => card.bank); | ||
| const personalDetails = usePersonalDetails(); | ||
| const participantsAutocompleteList = Object.values(personalDetails) |
There was a problem hiding this comment.
Good job, but still:
😆 but seriously NAB and if we need all this data then we need it
There was a problem hiding this comment.
Yea, I'm not sure there's a way around it. Maybe we can create a custom hook, useAutocomplete so the logic lives outside the component?
There was a problem hiding this comment.
I know this is still WIP so maybe you might be aware of these issues already, but I wanted to point them out anyways so we can get ahead and make sure they don't make to the final version of the PR:
- I think we should limit the number of suggestions to maybe 10 sorted alphabetically. You can see on the list that if I type from, a full list of all personalDetails comes up, which seems a bit useless.
- Cycling through the options should update the query string, e.g. when I press the arrow keys to select a different option, the query in the router should update to include the currently active suggestion.
- Selecting an autocomplete suggestion runs the query, but it drops the filter key. Note in the video how I selected
from:[email protected]but the final search dropped thefrom:part
Screen.Recording.2024-10-23.at.9.49.14.AM.mov
| ); | ||
| return; | ||
| } | ||
| default: { |
There was a problem hiding this comment.
NAB we can DRY the code above since they all run the same logic, just on different lists
There was a problem hiding this comment.
I know this code looks easily "DRYable", but with Mateusz PR these switch cases will become much more distinct. Let's not add conflicts to his branch, as he said in #51237 (comment) 😄. After merging these two PRs we could put up a small clean-up PR.
| return Array.from(uniqueCategoryNames); | ||
| } | ||
| return Object.values(singlePolicyRecentCategories ?? {}).map((category) => category); | ||
| } |
There was a problem hiding this comment.
NAB maybe we could DRY the logic in some of these functions too since the logic is 95% identical
There was a problem hiding this comment.
Same as in #51237 (comment)
|
|
@ikevin127 @luacmartins @SzymczakJ Because of that I would like to suggest any NAB's and other improvements are not done here, but I will instead do them in my PR 😁 it will greatly help me. |
It results in infinite loading, because it doesn't make sense from UI point of view(we cannot be on an Expense tab and on Chat tab on the same moment). On the other hand I think that we shouldn't limit the user, even though his query may produce something that doesn't make sense. Maybe we could provide some modal on SearchResultsPage that tells user that 'This query doesn't make sense'. WDYT @luacmartins
No autocomplete was added for input "category:X ," because of the limitations of our parser. Here is my solution for this: when user adds "," to add another autocomplete then the spaces before "," are trimmed so our parser can parse it properly. Please check it out @luacmartins @ikevin127 and tell me WDYT proposal.mov |
Yea, I agree that this is an issue, but it also happens on main today because a user can type that. So NAB for this PR. I think ultimately, we need to improve the parser to validate some of these keys, like type, status, sortBy, sortOrder, etc so we handle this more gracefully. NAB for now though
I like it! Thanks for fixing that! |
|
NAB I think we can get smarter with the suggested statuses and only suggest statuses that apply to the current type |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.56-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.56-9 🚀
|
Details
This PR adds autocomplete in SearchRouter for simple keys like categories, tags, etc.. Searching with 'in:' keyword is not yet fully supported(reportIDs instead of full names will be visible in text input)
Fixed Issues
$ #50942
$ #50947
$ #50957
$ #50952
$ #50953
$ #50954
$ #50955
$ #50956
$ #50948
$ #50945
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
androidweb.mov
iOS: Native
ios.mov
iOS: mWeb Safari
iosweb.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov