-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Better RHP view #67533
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
Better RHP view #67533
Conversation
|
🚧 @shawnborton has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Some quick notes:
|
|
The very past part of the RHP animation - it kind of quickly moves to it's final resting point and doesn't arrive there smoothly, almost like a sudden shift at the end: CleanShot.2025-07-31.at.17.06.02.mp4 |
8605dbb to
fa291aa
Compare
|
🚧 @shawnborton 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! 🧪🧪
|
|
It still feels very very choppy when you open up the RHP: CleanShot.2025-08-10.at.17.50.16.mp4 |
7939cac to
5a477ca
Compare
@shawnborton, can you share this report with me somehow? I can't find such case using my account |
|
Sure, should we run another test build too and then I can check my reports again? |
Yeah, I think that would be the best option. |
|
You can also look if there is anything else worth fixing |
c6adadc to
9b8ee8f
Compare
|
🚧 @shawnborton 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! 🧪🧪
|
|
The initial loading skeleton isn't as wide as the final RHP, so that's slightly jarring... not sure if we can do anything about that? CleanShot.2025-08-14.at.11.12.00.mp4 |
|
Any updates on this one? I would love to speed this one up if possible, thanks! |
|
@shawnborton We had a holiday on Friday in Poland. It was mentioned on Slack, but I forgot to mention it here I am currently working on the responsive pane for the 800px to 840px range. It should be ready soon. And about the PDFs. That's weird. It seems to work fine on my end. Is there anything else that could be different in our cases? Also, could you please try the same on the new account with a new PDF report?
I think I could try to implement an optimistic solution that will work if the user navigates from this specific page (expenses) and probably the other one (search/r) |
|
Started a new build @getusha are you up for this PR review and testing? |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
@adamgrzybowski can you please thing of more specific test cases - for different expense types, card, eReceipt, with or without receipt, distance etc |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
I don't think the background in the top right is a problem personally. The empty space on the left looks a bit weird, but I'm not sure if we wanna stretch it and then crop out some parts of the receipt. Curious to hear what @shawnborton envisioned. |
|
Same - I feel like maybe he had a solution in mind for that situation, though I can't remember exactly how he was handling it. Edit: Actually, looking at this prototype he posted in Slack, it looks like he has it growing/stretching with cropping up to a certain height, and then it looks fairly similar to what Jason is showing in his screenshot. |
…ipt-on-load Add onLoad prop to EReceipt
|
I just merged this PR that should ensure the expense details are anchored to the top #68932 btw |
Would we do that for regular chat too or just expenses? |
|
I think we'd only need to do it for chats that we purposely top-anchor, which would be expenses and reports. |
@shawnborton would you mind posting a mock of how you envision that layout on a super tall screen? (Like one Jason would use 😂) Just trying to get a sense of the ideal alignment in that not-so-ideal case, mostly in the bottom right corner. |
|
Ok cool! That's what I was expecting—but I wanted to make sure we were on the same page. Thanks! |
|
Nice, great improvement! |
|
@adamgrzybowski I believe we can clean this one up now as the last PR, right? |
|
Yeah no problem, I was not sure if you wanted to repurpose this one after merging main |
















Explanation of Change
Implementation of the wide RHP functionality to always show the receipt image on the wide layout.
Fixed Issues
$ #64025
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Verify if the receipt is displayed correctly on the wide screen
(You should do these steps for 5 different types of receipt image: empty, PDF, photo, distance, e-receipt)
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
Android: mWeb Chrome
iOS: Native
ios.mp4
iOS: mWeb Safari
iosWeb.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4