Add recent serches and recent chats in Search router#49457
Merged
luacmartins merged 34 commits intoExpensify:mainfrom Oct 2, 2024
Merged
Add recent serches and recent chats in Search router#49457luacmartins merged 34 commits intoExpensify:mainfrom
luacmartins merged 34 commits intoExpensify:mainfrom
Conversation
Contributor
|
This is looking really good so far! |
Contributor
Author
|
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
Contributor
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.
Contributor
Yes, we can add this in a follow up, but we should be able to filter reports while typing into the search router |
Contributor
Author
|
As it turns out that small refactor is needed I just need one day to make it C+ reviewable. |
Kicu
reviewed
Sep 25, 2024
Contributor
Kicu
left a comment
Kicu
left a comment
There was a problem hiding this comment.
I dropped some extra 1-2 comments for you, since you have a lot of linting errors.
I have not yet checked the complicated type casts - will do that later today
| } | ||
| }, SEARCH_DEBOUNCE_DELAY); | ||
| const onSearchChange = useCallback( | ||
| debounce((userQuery: string) => { |
Contributor
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
Contributor
|
rayane-d
approved these changes
Oct 2, 2024
Contributor
rayane-d
left a comment
rayane-d
left a comment
There was a problem hiding this comment.
LGTM and tests well. Let's address the above comments in follow-ups
luacmartins
approved these changes
Oct 2, 2024
Contributor
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
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. |
63 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.44-0 🚀
|
Contributor
|
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 |
Contributor
Author
|
Investigating |
Contributor
|
The reported performance regression can be ignored, see: |
Contributor
Author
|
@hannojg I've just made a PR with possible fix: #50143. Performance was probably worsened by putting |
Contributor
|
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 |
Contributor
Replied here: #50220 (comment) |
Contributor
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.0.44-12 🚀
|
7 tasks
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
10 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.
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