fix: save the scroll position in workspace list screen#77313
fix: save the scroll position in workspace list screen#77313francoisl merged 10 commits intoExpensify:mainfrom
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@truph01 Thanks for the PR. The android version does not work fine for me. 77313-android-hybrid-issue-001.mp4 |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Change is fine from a product perspective
|
On it now |
|
@rojiphil I've updated the PR. Here's the result when testing with an account that has more than 35 workspaces: Screen.Recording.2025-12-24.at.03.26.01.mov |
|
@rojiphil Please help re-review this PR once you have a chance. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp77313-android-hybrid-001.mp4Android: mWeb Chrome77313-mweb-chrome-001.mp4iOS: HybridApp77313-ios-hybrid-001.mp4iOS: mWeb Safari77313-mweb-safari-001.mp4MacOS: Chrome / Safari77313-web-chrome-001.mp4 |
rojiphil
left a comment
There was a problem hiding this comment.
Thanks @truph01 for the updates.
@francoisl Changes LGTM and works well too.
Over to you. Thanks.
| keyboardShouldPersistTaps="handled" | ||
| contentContainerStyle={styles.pb20} | ||
| onScroll={onScroll} | ||
| initialNumToRender={data.length} |
There was a problem hiding this comment.
Is this going to cause the list to render all items on mount? If so, it might have a performance impact, can we add a comment that explains why it's needed?
Also is there no way to make this dynamic based on the window height or something?
There was a problem hiding this comment.
There was a problem hiding this comment.
@rojiphil The initialNumToRender in the linked example
In my PR, initialNumToRender serves a different purpose: it’s required to handle scrolling correctly on the first render. Because of that, the number of items to render initially cannot be derived in the same way as in the referenced example.
There was a problem hiding this comment.
Ok fair. Can we still add a comment that explains why it's needed though please? Otherwise someone might try to remove it thinking it's unnecessary, and cause a regression.
There was a problem hiding this comment.
I added a comment. @rojiphil @francoisl, please help check.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/francoisl in version: 9.2.96-1 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.96-6 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.96-6 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.96-6 🚀
|
|
@truph01 @rojiphil @francoisl fyi we are reverting this PR because https://expensify.slack.com/archives/C05LX9D6E07/p1770725362096819?thread_ts=1770670428.191389&cid=C05LX9D6E07 I think that this feature is not work the overhead it brings for customer with many workspaces |
…-position-workspace-list [NoQA] Revert "Merge pull request #77313 from truph01/fix/76271"
|
🚀 Deployed to staging by https://github.com/francoisl in version: 9.3.17-0 🚀
|
|
Hi @truph01, QA team failed this PR on Web and Android with the original issue 1770794630964.77313A.mp41770794630966.77313w.mp4 |
|
@IuliiaHerets this was reverted, so that is expected |
|
Checked it off some mistake in the checklist |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.17-9 🚀
|
Explanation of Change
Implement scroll position preservation for
WorkspacesListPageusing the existingScrollOffsetContext.Fixed Issues
$ #76271
PROPOSAL: #76271 (comment)
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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.Recording.2025-12-11.at.14.22.41.mov
Android: mWeb Chrome
Screen.Recording.2025-12-11.at.11.35.05.mov
iOS: Native
Screen.Recording.2025-12-11.at.14.18.06.mov
iOS: mWeb Safari
Screen.Recording.2025-12-11.at.11.30.15.mov
MacOS: Chrome / Safari
Screen.Recording.2025-12-11.at.11.24.41.mov