Use autocomplete on Search Results Header#52568
Merged
luacmartins merged 13 commits intoExpensify:mainfrom Nov 26, 2024
Merged
Conversation
Kicu
force-pushed
the
kicu/autocomplete-polish
branch
from
5693688 to
19058b3
Compare
Contributor
|
@Kicu how's this PR coming along? |
Contributor
Author
|
@luacmartins I expect that I will open it for review today :) most of the heavy work is done and I have autocomplete components working on the Results page |
Kicu
force-pushed
the
kicu/autocomplete-polish
branch
from
19058b3 to
4a7922e
Compare
6 tasks
Kicu
force-pushed
the
kicu/autocomplete-polish
branch
from
4a7922e to
b7883d4
Compare
48 tasks
Kicu
commented
Nov 20, 2024
| * Given a filter name and its value, this function returns the corresponding ID found in Onyx data. | ||
| * Returns a function that can be used as a computeNodeValue callback for traversing the filters tree | ||
| */ | ||
| function getFindIDFromDisplayValue(cardList: OnyxTypes.CardList, taxRates: Record<string, string[]>) { |
Contributor
Author
There was a problem hiding this comment.
this is no longer needed because now substitutionsMap always keeps the connection between id and name
8 tasks
Contributor
|
Nice! Looking forward to this one |
Kicu
force-pushed
the
kicu/autocomplete-polish
branch
from
b7883d4 to
29ee84f
Compare
|
@rayane-djouah 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] |
Contributor
Author
|
For the reviewer: However I didn't do these yet, since I thought they might make the review tricker in case git treats them as completely new files 😛 |
Contributor
|
@rayane-djouah let's prioritize reviewing this one |
Contributor
Author
|
Also I would like to get some feedback from the UX team about how this list should look. 
VS 
CC @shawnborton |
Contributor
|
This is what we want the styles to look like: CleanShot.2024-11-20.at.14.05.00.mp4On mobile, we basically just use the whole view and add a close button to the top right: 384986249-40070501-1cec-4b7e-afef-fc242a389859.mp4Note that we have some overlap here with this upcoming issue too: #52317 |
Contributor
I Will review it this afternoon 👍 |
Contributor
Author
|
@shawnborton @luacmartins whoops... sorry we never discussed this on mobile 😅 In this PR code is not ready to work on mobile, as the input was never there from the beginning. I think the best course of action is to leave the mobile implementation for #52317 - it will fit nicely there. Either me or someone else from SWM will pick this one up shortly. On the other hand this PR adds a lot of logic and improves autocomplete behavior so I think it's best to not add any more scope to it. I will try to make the list look good enough for this PR. |
Contributor
|
That works for me, just want you to be aware of the upcoming changes so we aren't duplicating the work. And ideally you can at least get web/desktop looking like my video above. Thanks! |
Contributor
|
BUG: The suggestion box container height briefly exceeds the height of the suggestions Screen.Recording.2024-11-26.at.7.58.22.PM.mov |
Contributor
|
BUG: Clicking on an autocomplete suggestion does not automatically re-focus the input. android_mweb_chrome.mp4 |
Contributor
|
I think that #52568 (comment) and #52568 (comment) are not blockers |
Contributor
|
Checklist completed: #52568 (comment) |
Contributor
|
Agree that those are not blockers. Thanks for catching them though. @Kicu let's address them in the follow up |
luacmartins
approved these changes
Nov 26, 2024
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
github-actions
bot
added
the
DeployBlockerCash
Contributor
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
Contributor
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
Contributor
|
Hmm maybe this PR introduced a performance regression @Kicu |
Contributor
Author
|
@luacmartins I will look into that possible regression. I think for the code it is great that this one is merged. |
48 tasks
72 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.68-0 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.68-7 🚀
|
s77rt
reviewed
Feb 1, 2025
Comment on lines
+104
to
+107
| if (updatedUserQuery || textInputValue.length > 0) { | ||
| listRef.current?.updateAndScrollToFocusedIndex(0); | ||
| } else { | ||
| listRef.current?.updateAndScrollToFocusedIndex(-1); |
Member
There was a problem hiding this comment.
The updatedUserQuery || textInputValue.length > 0) is pretty much always true because textInputValue is stale and holds the previous value. This caused the focus not being cleared on clearing the text input.
Comment on lines
+132
to
+137
| if (query !== originalInputQuery) { | ||
| SearchActions.clearAllFilters(); | ||
| setTextInputValue(''); | ||
| setAutocompleteQueryValue(''); | ||
| setIsAutocompleteListVisible(false); | ||
| } |
Member
There was a problem hiding this comment.
If this condition is not met we should reset the text input value to queryText because the text input value can be changed back and forth to the original query e.g.
- Textinput value reflects the current search query
- Clear textinput
- Select previous value (from suggestion list)
Now after step 3, the query === originalInputQuery but the textinput is still empty.
Comment on lines
+245
to
+247
| onSubmit={() => { | ||
| submitSearch(textInputValue); | ||
| }} |
Member
There was a problem hiding this comment.
This callback should be called only if no item is focused. Otherwise on pressing Enter two handlers are executed: onSubmit (this one) and the onListItemPress which conflicts
Contributor
Author
|
@s77rt thanks for the comments 👍 |
Contributor
Contributor
Author
|
Oh I see, thanks. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
This PR is a followup to #51633 which added better handling of autocomplete ids. However we were missing this in the input on SearchResults page - this PR implements that.
A lot of refactors were needed so that we can reuse the code and ensure the same behavior of autocomplete lists both in SearchRouter and on the results page.
Here's how it looks
Note: this Input exists only on wide/desktop version.
main changes
SearchRouterList, which is now used in two places;SearchRouterbecause we don't want it in the results page;SearchPageHeaderInput: it basically does the same things asSearchRouterbut without the "routing" part 😅 for the Results Page;SearchRouterListbecause they became too complexSubstitutionsMap, so now there is no need for function likefindIDFromDisplayValueold bugs fixed
OptionsUtils.getFilteredOptionsso sorting is better and same as in the Advanced Filters selectorscardIDseem to display last 4 digits (it's hard for me to test every edge case as I only have fake card data)currentUseroption now included infrom/toFixed Issues
$ #50976
PROPOSAL:
Tests
from:,to:,in:, 'cardID:,taxRate:`Offline tests
QA Steps
from:,to:,in:, 'cardID:,taxRate:`PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)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: mWeb Chrome
rec-andr-autocomplete.mp4
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
rec-web-autocomplete.mp4
MacOS: Desktop