[Better Expense Reports] Prevent TransactionPreview incomplete viewability#61069
Merged
mountiny merged 10 commits intoExpensify:mainfrom May 9, 2025
Merged
Conversation
…-fork into Guccio163/wideViewNarrowCarousel
melvin-bot
bot
requested review from
DylanDylann
and removed request for
a team
|
@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] |
trjExpensify
requested review from
allgandalf
and removed request for
DylanDylann
Contributor
|
🚧 @shawnborton has triggered a test app build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
Contributor
|
Hmm now the default width of the report preview card is way too small. This is what I see with a wide screen: |
Contributor
|
We still want it to look like this on wider screens: |
Contributor
Author
|
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! 👋 |
Contributor
|
hmm, seems like we are holding this PR until next week..... |
…-fork into Guccio163/wideViewNarrowCarousel
Contributor
|
@Guccio163 are you back from OoO? |
Contributor
Author
|
yup, I'm working on different issue together with this one right now, but I can prioritise this if it's urgent |
Contributor
@Guccio163 what's the other issue for ref? |
Contributor
Author
|
FYI local fixing two-way scroll since it should be a quick fix |
…-fork into Guccio163/wideViewNarrowCarousel
Contributor
Author
|
Narrow issue for single transactions fixed, take a look if it works for you @shawnborton 👌 |
Contributor
|
Going to run a new test build now 🚀 |
Contributor
|
🚧 @shawnborton has triggered a test app build. You can view the workflow run here. |
Contributor
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
Contributor
|
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. |
Contributor
Author
|
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 |
Contributor
|
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. |
Contributor
Author
|
Ah, right, got it! Will add in a moment 🔜 |
Contributor
|
@Guccio163 you can update it today? I will review the changes tomorrow morning |
Contributor
Author
|
@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 |
Contributor
|
Yeah, I think that's totally fine! |
Contributor
|
@Guccio163 lets fix conflicts |
…-fork into Guccio163/wideViewNarrowCarousel
…-fork into Guccio163/wideViewNarrowCarousel
Contributor
Author
|
@allgandalf Ready 🚀 |
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / SafariScreen.Recording.2025-05-09.at.12.06.21.PM.movMacOS: DesktopScreen.Recording.2025-05-09.at.12.09.28.PM.mov |
allgandalf
approved these changes
May 9, 2025
mountiny
approved these changes
May 9, 2025
src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewContent.tsx
Show resolved
Hide resolved
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
github-actions
bot
added
the
DeployBlockerCash
Contributor
Performance Comparison Report 📊 (1/3)Significant Changes To Duration
Show details
|
Contributor
Performance Comparison Report 📊 (2/3)Meaningless Changes To Duration (1/2)Show entries
Show details
|
Contributor
Performance Comparison Report 📊 (3/3)Meaningless Changes To Duration (2/2)Show entries
Show details
|
Contributor
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
Contributor
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.45-0 🚀
|
Contributor
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.45-0 🚀
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 expensesis 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 Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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: mWeb Safari
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov