Move pre-selected list items to top of list throughout the app#69655
Move pre-selected list items to top of list throughout the app#69655lorretheboy wants to merge 36 commits intoExpensify:mainfrom
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
lorretheboy
force-pushed
the
fix/69184
branch
from
afc9e17 to
e3c938e
Compare
| ); | ||
|
|
||
| const searchResults = searchOptions(searchValue, countries); | ||
| const searchResults = searchOptions(searchValue, countries, true); |
There was a problem hiding this comment.
We just need to pass shouldMoveSelectedToTop: true here since we auto navigate back after user selects country
melvin-bot
bot
requested review from
linhvovan29546
and removed request for
a team
|
@linhvovan29546 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] |
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
We discussed this change in Slack 👍 CC'ing @Expensify/design for a look here too!
|
Big fan of this. Let's run an adhoc build when it's ready for testing? |
linhvovan29546
left a comment
linhvovan29546
left a comment
There was a problem hiding this comment.
The change looks good. Could you please check some minor comments?
| function CountrySelectorModal({isVisible, currentCountry, onCountrySelected, onClose, label, onBackdropPress}: CountrySelectorModalProps) { | ||
| const {translate} = useLocalize(); | ||
| const [searchValue, debouncedSearchValue, setSearchValue] = useDebouncedState(''); | ||
| const [hasUserInteracted, setHasUserInteracted] = useState(false); |
There was a problem hiding this comment.
Can we use useRef instead of useState?
There was a problem hiding this comment.
Hmm looks like we can't as useRef doesn't trigger re-render. Correct me if I'm wrong @linhvovan29546
There was a problem hiding this comment.
The onCountrySelected function triggered re-render. I think it's better to use useRef instead of useState, since state updates are asynchronous and may cause unnecessary additional re-renders, while a single re-render is sufficient in this case. Additionally, calling hasUserInteracted before setCurrentCountry helps ensure the value is set correctly before other changes occur. Using a ref guarantees that the value is updated immediately and avoids potential synchronization issues.
There was a problem hiding this comment.
@linhvovan29546 I have tried and no luck here, but useRef doesn't trigger re-render and also the linter is warning that we shouldn't access ref.current during render. Could you pls draft your solution for me if possible just in case I did something wrong? Thanks
There was a problem hiding this comment.
Ah, I just double-checked. I think the current useState is fine
| ); | ||
|
|
||
| const searchResults = searchOptions(debouncedSearchValue, countries); | ||
| const searchResults = searchOptions(debouncedSearchValue, countries, !hasUserInteracted); |
There was a problem hiding this comment.
Please use useMemo as well, and apply the same in other places.
src/libs/searchOptions.ts
Outdated
| * @returns An array of options with selected items at the top | ||
| */ | ||
| function moveSelectedOptionsToTop<T extends {isSelected?: boolean}>(options: T[]): T[] { | ||
| const selected: T[] = []; |
There was a problem hiding this comment.
After reviewing this issue, I see that we only have single selected item. So instead of using a for loop, we can use findIndex with splice like this(correct me if i'm wrong)
const result = [...options]; // Create a shallow copy to avoid mutation
const index = result.findIndex(option => option.isSelected);
if (index > 0) {
const [item] = result.splice(index, 1);
result.unshift(item); // Move selected item to top
}
return result;
There was a problem hiding this comment.
hmm I think u are correct, we only have single selection
|
@linhvovan29546 I have resolved & replied your comments. Pls help check once u having time. Thanks |
|
The failed test is not related to this PR. Re-run test... |
|
Not related to Country selection list but should we apply the same behavior to other selections such as State selection,... 
|
Yeah, I think the goal here is that all lists should behave the same way. |
|
Agree! |
|
Right, I agree. I'm a little worried by the question, tbh. The PR title says "Move pre-selected list items to top of list throughout the app" after all, you'd expect a code solution to be higher up than the country selector modal in a select list component or something. |
|
Hi @trjExpensify, sorry for that question, I just want to confirm that my changes won't be redundant. I was a little bit confusing when making the PR when I read again this note in OP, though in my proposal I suggest a global changes I will note and be more careful when reading the OP next time 
|
|
Don't be sorry! Definitely encouraged and better to ask the question than not. Thanks! |
|
Oh yeah I'd be into something like that Shawn! |
|
Agree with all your comments @shawnborton 👍 The "above X" thing makes sense especially when you look at lists like |
|
Update: I am on the way to include the threshold for "move to the top" behavior. I took longer time than expected. I will give you all update once finish it. Thanks |
|
Still on it. I will try to get it ready for review before Monday |
|
Hi @linhvovan29546, I have updated the code to address new requirements. Ps: Should we consider to hold on reviewing this issue, this I notice we would have a major refactoring here? Not |
|
I'll review this tomorrow. |
|
@lorretheboy I see that this affects the issue as well. We have updated it to use the useSearchSelector hook, right? |
|
yes @linhvovan29546, it is also what I concerned |
|
What's the latest here? We now have conflicts. Let's try to get this over the finish line, we've had it open for quite a while! |
|
We have some sub-issues here: #66615, which conflict with this PR. I suggest we hold off on tis until these issues are resolved. 
|
|
@lorretheboy In the meantime, I think we can also update this PR to use |
linhvovan29546
left a comment
linhvovan29546
left a comment
There was a problem hiding this comment.
@lorretheboy I's added a few comments and am continuing to test this.
| const isPolicyExpenseChat = participant?.isPolicyExpenseChat ?? false; | ||
| return isPolicyExpenseChat ? getPolicyExpenseReportOption(participant, reportAttributesDerived) : getParticipantsOption(participant, personalDetails); | ||
| }), | ||
| data: [chatOptions.userToInvite] |
There was a problem hiding this comment.
@lorretheboy How did you test this? I don't know the steps to get userToInvite
There was a problem hiding this comment.
We can just type a completely new email, it will suggest new user --> Press to it @linhvovan29546
There was a problem hiding this comment.
Bug: Can't select the user invite
Screen.Recording.2025-10-20.at.17.33.29.mov
| const firstRenderRef = useRef(true); | ||
| const [betas] = useOnyx(ONYXKEYS.BETAS, {canBeMissing: false}); | ||
| const [invitedEmailsToAccountIDsDraft] = useOnyx(`${ONYXKEYS.COLLECTION.WORKSPACE_INVITE_MEMBERS_DRAFT}${route.params.policyID.toString()}`, {canBeMissing: true}); | ||
| const initialSelectedAccountIDs = useMemo(() => new Set(invitedEmailsToAccountIDsDraft ? Object.values(invitedEmailsToAccountIDsDraft) : []), [invitedEmailsToAccountIDsDraft]); |
There was a problem hiding this comment.
@lorretheboy Bug: The 'Contacts' section is missing
|
Hi all, for your update, I will back on this PR today. It looks like the |
|
@lorretheboy how is it looking? |
|
@mountiny I resolved conflicts and fixing bugs now. I will update the status EOD |
|
My original solution is not working anymore in several pages as we did refactoring to |
|
Sorry all, I still couldn't come up with new solution to make it work after the selection list refactoring. I think we can reassign in case we need to do it with priority |
|
@srikarparsi seems like you are assigned to the linked issue, we might need a new proposal for this issue |
|
Sounds good, let me take a deeper look at this tomorrow. Thanks for the tag @mountiny |
|
@lorretheboy Could you please close this PR? |
8 participants
Explanation of Change
Fixed Issues
$ #69184
PROPOSAL: #69184 (comment)
Tests
Test 1: Country selection in Update profile address
AddressCountryTest 2: Country selection in Company cards
Test 3: Country selection in Wallets
Test 4: Advanced filters
Note that the selected value will be on top and after that it should remain its own position for every time user selects new value
Offline tests
QA Steps
Similar to Tests step
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
https://drive.google.com/file/d/1K4Siz1poSNLtHJreaae0Y2YxFrymyvnN/view?usp=sharing
Android: mWeb Chrome
https://drive.google.com/file/d/1-mJhGIOytoh_B1kD7Gaplg4tyAfIUvQW/view?usp=sharing
iOS: Native
https://drive.google.com/file/d/1LTwiHLJHkAY0_6jDD4MaPpu8Fp3pULt0/view?usp=sharing
iOS: mWeb Safari
https://drive.google.com/file/d/181U3oF5fzUJdt61_Fow0adjbGsRUJ5Y3/view?usp=sharing
MacOS: Chrome / Safari
https://drive.google.com/file/d/1GxDmV_gK62YgAn1rU__LsghUNihYT7Tz/view?usp=sharing
MacOS: Desktop
https://drive.google.com/file/d/16XMa68hwIiIgfBZ8vm8zpKK444LHlmbU/view?usp=sharing