Repository navigation
Move pre-selected option to top in shared SelectionScreen (accounting selectors) - #101719
Conversation
…d clarify default key matching
… and update related tests
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@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] |
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required on this one.
|
@linhvovan29546 Could you please check this if it is a bug and should I fix it:
After clearing the search, the list automatically scrolls toward the new selected item (Test Expense Account 05). screenrecord.mov |
|
@linhvovan29546, Gentle bump. thanks. |
I don’t think this is the bug. |
|
@MelvinBot Review |
Code reviewThe approach looks right: pinning before filtering in 1. Multi-select pages pin only the first pre-selected item. const initialSelectedValues = useInitialValue(() => data.filter((item) => item.isSelected).map((item) => item.value));
const orderedData = moveInitialSelectionToTop(data, initialSelectedValues);This also uses the existing 2. 3. Small fix to the comment at Checked and fine:
|
| const shouldShowTextInput = supplierOptions.length >= CONST.STANDARD_LIST_ITEM_LIMIT; | ||
|
|
||
| // Pin the selected supplier to the top of the full list before filtering, so it stays pinned while searching. | ||
| const orderedSupplierOptions = useMemo(() => moveInitialSelectionToTop(supplierOptions, currentContactID ? [currentContactID] : []), [supplierOptions, currentContactID]); |
There was a problem hiding this comment.
Please remove useMemo. The DynamicXeroNonReimbursableDefaultContactSelectPage page has been automatically memoized by the React Compiler.
…d items at the top and update related tests
| const [searchValue, setSearchValue, filteredData] = useSearchResults(data, (item, searchInput) => tokenizedSearch([item], searchInput, () => [item.text ?? '', item.value]).length > 0); | ||
| // Freeze the items selected when the list opened so they stay pinned while staged selections change and the search | ||
| // filters the list. All pre-selected values are kept so multi-select lists pin every one, not just the first. | ||
| const initialSelectedValues = useInitialValue(() => data.filter((item) => item.isSelected).map((item) => item.value)); |
There was a problem hiding this comment.
Why don’t we use useInitialSelection?
There was a problem hiding this comment.
@linhvovan29546 useInitialSelection always calls useFocusEffect internally, which would add a navigation-context dependency to this shared hook (used by ~40 selectors) and break its renderHook unit test. We also don't want its resetOnFocus behavior here: the hook only sees the live isSelected, so re-snapshotting on focus would re-pin the staged selection — the exact jump we're preventing. Without resetOnFocus it's just useInitialValue with extra weight, so useInitialValue (which SingleSelect/MultiSelect already use) is the lighter, correct fit — freeze once at mount.
|
@robertjchen, could you please feel free to review the code, but hold off on merging for now? I’m doing another round of testing. Thanks! |
|
@linhvovan29546, could you please take another look, this is done from my side. thanks. |
|
🚧 robertjchen has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/robertjchen in version: 9.5.15-1 🚀
|
|
No help site update needed. This PR only changes list order, so the pre-selected option now appears at the top of long, searchable accounting selectors. No article under |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.5.15-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Part of the "move pre-selected list items to top" work
This PR adds the "pin the pre-selected item to the top" behavior directly in the shared
SelectionScreencomponent, so every selector built on it gets the behavior from a single place instead of editing each page individually.Fixed Issues
$ #69184
PROPOSAL: #69184 (comment)
Tests
These steps verify it on the QuickBooks Online → Advanced → foreign-currency expense account selector (the reachable QBO one), but the same behavior applies to all ~24 searchable accounting selectors since they share this code path.
**Setup (DevTools console)** — unlocks the FX selector and seeds a long, pre-selected list
(Run this right before testing and stay in the app — a server reconnect can overwrite these optimistic values.)
Testin the search box to filter the list, then clear the search.Offline tests
Same as Test
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as Test
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
checklist.mov
Android: mWeb Chrome
iOS: Native
checklist.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
check.list.mov