fix: inconsistent category order display in list & expense page#61656
Merged
dangrous merged 22 commits intoExpensify:mainfrom May 30, 2025
Merged
fix: inconsistent category order display in list & expense page#61656dangrous merged 22 commits intoExpensify:mainfrom
dangrous merged 22 commits intoExpensify:mainfrom
Conversation
|
@ahmedGaber93 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] |
51 tasks
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
|
Contributor
|
@daledah The sort work with me only while searching, but the default list is not sorted, do you face this issue? 20250512040420353.mp4 |
Contributor
Author
|
@ahmedGaber93 I don't think the screenshot you sent is the correct one. In what page did you encountered the list not sorting bug? |
Contributor
|
In workspace categories page 20250514121510432.mp4 |
Contributor
|
@daledah Have you been able to identify the issue? |
Contributor
Author
|
@ahmedGaber93 I'm looking on it. Will give an update soon. |
Contributor
Author
|
@ahmedGaber93 I updated, it's actually a regression from my previous PR. |
8 tasks
src/hooks/useSearchResults.ts
Outdated
Comment on lines
+18
to
+19
| const normalizedSearchQuery = inputValue.trim().toLowerCase(); | ||
| const filtered = normalizedSearchQuery.length ? data.filter((item) => filterData(item, normalizedSearchQuery)) : data; | ||
| if (prevInputValueRef.current === inputValue) { | ||
| setResult(filtered); | ||
| return; | ||
| } | ||
| prevInputValueRef.current = inputValue; | ||
| startTransition(() => { | ||
| const normalizedSearchQuery = inputValue.trim().toLowerCase(); | ||
| const filtered = normalizedSearchQuery.length ? data.filter((item) => filterData(item, normalizedSearchQuery)) : data; |
Contributor
There was a problem hiding this comment.
@daledah Can you add more details about this change?
Contributor
Author
There was a problem hiding this comment.
This was added before when I was trying to fix a performance issue, but it turns out not working as expected because there are cases where we don't sort the items. So I reverted the changes back to initial implementation.
Contributor
|
Hmm! Do you know why sorting change from web to mobile? Web result is lowercase first "a, A" and mobile result is uppercase first "A, a". web
iOS native and mWeb
Also sorting change between categories and taxes in the same platform "iOS" iOS Taxes
iOS Categories
|
Contributor
Author
|
@ahmedGaber93 Can you check again if this demo is the correct order we want? cc @dangrous @shawnborton 
|
Contributor
Author
|
@ahmedGaber93 I updated. Should works well now 
|
Contributor
|
@daledah Thanks for update! The order looks well on all places except "Select category" on money request ("a" is displayed before "A").
|
Contributor
Author
|
@ahmedGaber93 I updated. |
dangrous
requested changes
May 27, 2025
Contributor
dangrous
left a comment
dangrous
left a comment
There was a problem hiding this comment.
two test comments, otherwise looks good!
Contributor
|
aw shoot conflicts, then we should be good to go |
Contributor
Author
|
@dangrous Sorry for the delay, conflicts resolved, we're ready to merge 👍 |
dangrous
approved these changes
May 30, 2025
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 📊 (1/3)Significant Changes To Duration
Show details
|
Contributor
Performance Comparison Report 📊 (2/3)Meaningless Changes To Duration (1/2)Show entries
Show details
|
Contributor
Performance Comparison Report 📊 (3/3)Meaningless Changes To Duration (2/2)Show entries
Show details
|
Contributor
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
Contributor
|
🚀 Deployed to staging by https://github.com/dangrous in version: 9.1.55-0 🚀
|
Contributor
|
🚀 Deployed to staging by https://github.com/dangrous in version: 9.1.56-2 🚀
|
Contributor
|
🚀 Deployed to staging by https://github.com/dangrous in version: 9.1.58-0 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.1.58-4 🚀
|
8 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
5 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
Fixed Issues
$ #61156
PROPOSAL: #61156 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Screen.Recording.2025-05-08.at.22.07.44.mov
Android: mWeb Chrome
Screen.Recording.2025-05-08.at.22.08.19.mov
iOS: Native
Screen.Recording.2025-05-08.at.22.10.38.mp4
iOS: mWeb Safari
Screen.Recording.2025-05-08.at.22.11.32.mp4
MacOS: Chrome / Safari
Screen.Recording.2025-05-08.at.22.12.21.mov
MacOS: Desktop
Screen.Recording.2025-05-08.at.22.22.28.mov