Add recent serches and recent chats in Search router #49457
Merged
Conversation
This file contains 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
|
This is looking really good so far! |
|
Sorry for the slow pace, I was working shorter hours today 😅 I asked @Kicu for internal review but he will complete it next week, as he's already gone. |
Kicu
approved these changes
Sep 23, 2024
There was a problem hiding this comment.
I tested and looks really good.
I found one small bug and a bunch of small improvements with naming and comments, so please check them and update.
Also question/possible bug:
Should recent reports also be filterable? because right now if I use chat finder, I can find different chats there, but in your PR they stay static.
We can of course add this in a separate PR, but I think if the plan is to completely remove Chat finder, then we should reimplement all its functionalities here.
Yes, we can add this in a follow up, but we should be able to filter reports while typing into the search router |
|
As it turns out that small refactor is needed I just need one day to make it C+ reviewable. |
Kicu
reviewed
Sep 25, 2024
| } | ||
| }, SEARCH_DEBOUNCE_DELAY); | ||
| const onSearchChange = useCallback( | ||
| debounce((userQuery: string) => { |
There was a problem hiding this comment.
@SzymczakJ
this construct useCallback(debounce(... will not pass our linting. I know because I faced this problem in the past.
Feel free to read more about this lint rule.
The solution will be either to drop useCallback for now, and make sure the function inside is not too computation-heavy, or eslint-ignore this line.
Alternatively check this article: https://kyleshevlin.com/debounce-and-throttle-callbacks-with-react-hooks/
For the record I think this warning is more like:
eslint cannot figure out what this function argument are, so it shouts a warning
than
this is bad react code
48 tasks
rayane-djouah
suggested changes
Oct 2, 2024
Comment on lines
+136
to
+138
| if (!currentQuery?.inputQuery && recentSearchesData && recentSearchesData.length > 0) { | ||
| sections.push({title: translate('search.recentSearches'), data: recentSearchesData}); | ||
| } |
There was a problem hiding this comment.
We are currently excluding recent search options when a query is typed, but instead, we should retain them and filter them based on the typed query.
There was a problem hiding this comment.
@rayane-djouah we decided to just remove recent searches from the list as soon as the user starts typing. Reason is that the list only ever has 5 entries and filtering the list is a bit overkill.
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid_native.mp4Android: mWeb Chromeandroid_mweb_chrome.mp4iOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-10-02.at.20.27.58.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-10-02.at.20.16.17.mp4MacOS: Chrome / SafariScreen.Recording.2024-10-02.at.8.07.14.PM.movMacOS: DesktopScreen.Recording.2024-10-02.at.8.10.18.PM.mov |
|
rayane-djouah
approved these changes
Oct 2, 2024
luacmartins
approved these changes
Oct 2, 2024
|
✋ 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
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
63 tasks
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.44-0 🚀
|
|
Cc @SzymczakJ could you please take a look at the performance regression above? I'm not sure if it came from this PR, but we should investigate |
|
Investigating |
|
The reported performance regression can be ignored, see: |
|
@hannojg I've just made a PR with possible fix: #50143. Performance was probably worsened by putting |
|
Ah, i didn't realise that the component was rendered so high up in the tree. yes, agree, its probably likely due to that! |
luacmartins
removed
the
DeployBlockerCash
6 tasks
6 tasks
|
This PR is failing because of issue #50220 |
Replied here: #50220 (comment) |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.0.44-12 🚀
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Details
This PR adds recent chats and recent searches to search router.
Fixed Issues
$ #49120
$ #49116
PROPOSAL:
Tests
Offline tests
QA Steps
same as tests
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