-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Make SplitExpensePage use new SelectionList
#75289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make SplitExpensePage use new SelectionList
#75289
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
|
||
| // Delay scrolling by 100ms to allow the keyboard to open. | ||
| // This ensures FlashList calculates the correct window size. | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solution is only temporary because KeyboardAwareScrollView does not currently work well with FlashList v2.
The average time it takes for the keyboard to fully open is 300 ms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove setTimeout in BaseSelectionListWithSections? We don't have setTimeout, so if we can still reproduce this issue in staging, we don't need to add timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I understand 🤔
With the previous SelectionListWithSections we used a different approach that involved a custom hook to accurately calculate the window size (useDisplayFocusedInputUnderKeyboard) for mWeb and KeyboardAwareScrollView for natives.
However the current solution doesn't work well cause KeyboardAwareScrollView is not very compatible with FlashList v2.
So the core problem is a FlashList timing issue:
- it calculates the available window size too early
- it attempts to scroll to the target element before the on-screen keyboard has finished opening
- the calculated scrolling position is incorrect
Adding the 300 ms delay (which is the average time for the keyboard to appear on most Android and iOS phones) ensures that FlashList waits long enough to calculate the final, correct screen size (with the keyboard fully visible) before executing the scroll, allowing it to move to the proper location
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry @zfurtak, I mean
Can we remove setTimeout? In BaseSelectionListWithSections we don't have setTimeout, so if we can still reproduce this issue on staging, we don't need to add a timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On staging it's not reproducible because the other workaround works there, with FlashList it doesn't work properly 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I got it
|
@zfurtak code looks good. Can you please merge main? Then I'll test it |
|
@dukenv0307 merged and ready :) |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-01-08.at.09.47.46.movAndroid: mWeb ChromeScreen.Recording.2025-12-16.at.19.16.12.moviOS: HybridAppScreen.Recording.2026-01-08.at.09.50.55.moviOS: mWeb SafariScreen.Recording.2025-12-16.at.19.15.12.movMacOS: Chrome / SafariScreen.Recording.2025-12-16.at.19.13.11.mov |
|
Bug: the focused input scrolls up a bit then scrolls to bottom this PR Screen.Recording.2025-12-16.at.19.18.16.movScreen.Recording.2025-12-16.at.19.18.33.movmain/staging: Screen.Recording.2025-12-16.at.19.20.37.movScreen.Recording.2025-12-16.at.19.20.53.mov |
grgia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running a test build
|
No product UI change so removing product reviewer |
grgia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zfurtak would you merge main here and address lint for final review please 🙇
|
I'm back and on it 😊 |
2be0c8d to
c26c891
Compare
|
Should be better now, @dukenv0307 could you retest it? |
|
🚧 @grgia has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Explanation of Change
Fixed Issues
$ #72972
PROPOSAL:
Tests
More->SplitOffline 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))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.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-20251208-160200.2.mp4
Android: mWeb Chrome
screen-20251208-161000.mp4
iOS: Native
Screen.Recording.2025-12-08.at.16.21.39.mov
iOS: mWeb Safari
Screen.Recording.2025-12-08.at.16.19.45.mov
MacOS: Chrome / Safari
Screen.Recording.2025-11-17.at.15.44.53.mov
MacOS: Desktop