-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Provide autocomplete suggestions in SearchRouterList #51237
base: main
Are you sure you want to change the base?
Provide autocomplete suggestions in SearchRouterList #51237
Conversation
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a quick glance the logic and data flow looks good 👌
I hope that in future we can somehow split the logic so that SearchRouter
does not become a "god component"
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is possible to use default argument instead, and do
getAutoCompleteTagsList(allPoliciesTagsLists: OnyxCollection<PolicyTagLists> = {}, policyID?: string)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very small stuff, feel free to open PR when you're ready.
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@luacmartins maybe @rayane-djouah will be a better pick for reviewer since he has so much context here? Of course if you don't mind @ikevin127 🙇 |
Both @rayane-djouah and @ikevin127 have been involved and reviewing Search PRs for a while, so they both have context and should be able to handle these PRs. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
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 Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.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