-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix: ActionSheetAwareScrollView not working with maintainVisibleContentPosition
#68648
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
fix: ActionSheetAwareScrollView not working with maintainVisibleContentPosition
#68648
Conversation
|
🚧 @rlinoz has triggered a test Expensify/App build. You can view the workflow run here. |
|
@chrispader I am not sure why I am being tagged here |
sorry wrong C+, my mistake |
This comment has been minimized.
This comment has been minimized.
|
Please conduct complete testing for this and check how content is handled for:
|
|
@rlinoz @ishpaul777 this PR is needed for #51366. Is it possible to get this reviewed and tested? |
|
@chrispader can you merge main again, please? I think we fixed the iOS build recently. |
…ith-maintainVisibleContentPosition
@rlinoz done! |
|
🚧 @rlinoz 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, Desktop, and Web. Happy testing! 🧪🧪
|
|
I am not sure if this is a regression from this PR or not, but the linked message is not visible on long press after deeplinking to it Screen.Recording.2025-08-19.at.11.15.40.movOther than that everything else seems fine. |
|
cc: @chrispader on the above |
…ith-maintainVisibleContentPosition
@rlinoz i can also reproduce this on the latest app version from the App Store. This is definitely a bug, but i'd suggest that i fix this in a follow-up PR to not HOLD this change, as it's not causing this issue: ScreenRecording_08-22-2025.16-56-20_1.MP4 |
|
hey @ishpaul777 can you help review this one as well as part of #35011 ? |
|
hey @rlinoz 👋, I was mostly OOO this week but just got online. I’ll make sure to catch up on this and prioritize (today or over weekend) 🙇 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppios specific change not required Android: mWeb Chromeios specific change not required iOS: HybridAppScreenRecording_08-24-2025.23-36-23_1.MP4iOS: mWeb Safariios specific change not required MacOS: Chrome / Safariios specific change not required MacOS: Desktopios specific change not required |
ishpaul777
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.
lgtm!
|
✋ 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/rlinoz in version: 9.1.99-0 🚀
|
|
I think we might have caused this regression #69253 |
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.1.99-11 🚀
|
@rlinoz @ishpaul777
Explanation of Change
This PR fixes an issue on iOS with
ActionSheetAwareScrollViewin combination withmaintainVisibleContentPosition. In #51366, when we start rendering the report at some specific comment,maintainVisibleContentPositionis not working because theScrollViewonly contains one element.This PR adapts the
ActionSheetAwareScrollViewto apply the extra padding on the container rather than inside the scroll content.Fixed Issues
$ #35011
PROPOSAL:
Tests
Test
ActionSheetAwareScrollView:Test
maintainVisibleContentPosition:Offline tests
None needed.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as in Tests.
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
iOS: Native
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-08-18.at.12.57.28.mp4