-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[Better Expense Reports] Prevent TransactionPreview incomplete viewability #61069
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 Expense Reports] Prevent TransactionPreview incomplete viewability #61069
Conversation
…-fork into Guccio163/wideViewNarrowCarousel
@DylanDylann 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] |
🚧 @shawnborton has triggered a test app build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
Oh yeah - I just investigated this, and normally Carousel's width is calculated basing on available space and TransactionPreview's width - since it was constant, there was no problem. Right now, when we try to update TransactionPreview's width basing on Carousel's width, it falls into the loop: TransactionPreview is short by default, it doesn't stretch the Carousel even if there is space available and if Carousel doesn't stretch, neither does TransactionPreview - that's why it stays of the default narrow view and doesn't stretch to the expected 303px. I'm finishing my work for this week, so I'll get bach to it on Monday! 👋 |
hmm, seems like we are holding this PR until next week..... |
…-fork into Guccio163/wideViewNarrowCarousel
@Guccio163 are you back from OoO? |
yup, I'm working on different issue together with this one right now, but I can prioritise this if it's urgent |
@Guccio163 what's the other issue for ref? |
FYI local fixing two-way scroll since it should be a quick fix |
…-fork into Guccio163/wideViewNarrowCarousel
Narrow issue for single transactions fixed, take a look if it works for you @shawnborton 👌 |
Going to run a new test build now 🚀 |
🚧 @shawnborton has triggered a test 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! 🧪🧪 |
Looking good! Only minor thing I caught was that I would expect the button to be full width when we use the carousel at this screen size. Notice how the view button above it is full width? That's what we want: We don't want to change the max-width for normal screen sizes though, let's not touch that existing behavior. |
I think previously we decided that the button will be the width of the TransactionPreview, are we sure we want to change this? It shouldn't be a big thing to do, but I'm making sure ☝️ The button above is ReportPreview's size, because the one above is single-transaction, so TransactionPreview has 100% width |
Yeah, we want the button to match what is showing above. It follows the same thing we do for mobile, when we use a full-width button even if we have multiple previews in the carousel. |
Ah, right, got it! Will add in a moment 🔜 |
@Guccio163 you can update it today? I will review the changes tomorrow morning |
@shawnborton Okay I have a solution you are asking for, but currently it snaps between situation where we want to use TransactionPreview's width and carousel's width - does it work for you? Screen.Recording.2025-05-08.at.10.11.11.mov |
Yeah, I think that's totally fine! |
@Guccio163 lets fix conflicts |
…-fork into Guccio163/wideViewNarrowCarousel
…-fork into Guccio163/wideViewNarrowCarousel
@allgandalf Ready 🚀 |
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / SafariScreen.Recording.2025-05-09.at.12.06.21.PM.movMacOS: DesktopScreen.Recording.2025-05-09.at.12.09.28.PM.mov |
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.
Nab comments
src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewContent.tsx
Show resolved
Hide resolved
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊 (1/3)Significant Changes To Duration
Show details
|
Performance Comparison Report 📊 (2/3)Meaningless Changes To Duration (1/2)Show entries
Show details
|
Performance Comparison Report 📊 (3/3)Meaningless Changes To Duration (2/2)Show entries
Show details
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.45-0 🚀
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.45-0 🚀
|
Explanation of Change
Currently, when screen is in wide setting, the
TransactionPreview
's width is constant at 303px, but it is possible for available space to be less than that - that disturbs the comfortable use of the Carousel and readability of the expenses. It might also lead to navigation issues.Proposed solution is making
TransactionPreview
's width adaptable to narrowing screen's width, when it is too small to fit default 303px version. In described scenario, alsoX expenses
is hidden to enhance report's title readability, but is still visible when TransactionPreview's width reaches default.Before:

After:

Fixed Issues
$ #61161 (comment)
PROPOSAL:
Tests
Offline tests
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov