-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[Better Expense Report View] Expense Report Preview #58479
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 Report View] Expense Report Preview #58479
Conversation
…-fork into Guccio163/MoneyRequestReportPreview
…-fork into Guccio163/MoneyRequestReportPreview
Hi, I'm creating this draft so you can overview how it goes forward; there are still some things to be done: retrieving correct RBR messages, working left/right arrows, mobile navigation dots and standardising the styling - they are still in the There are also 2 problematic issues, but they can potentially be handled a little bit later:
I consulted both od these issues with colleagues from SWM and we couldn't find a plausible solution, so I left them for now not to spend more time on that. Feel free to let me know what do you think, just keep in mind I'm aware that current state is still some way from finished ;) |
…-fork into Guccio163/MoneyRequestReportPreview
…-fork into Guccio163/MoneyRequestReportPreview
…-fork into Guccio163/MoneyRequestReportPreview
…-fork into Guccio163/MoneyRequestReportPreview
Hi, I'm in a process of restyling and I stumbled over this part of code in
From what I've seen the common rule is to add |
…-fork into Guccio163/MoneyRequestReportPreview
…-fork into Guccio163/MoneyRequestReportPreview
…-fork into Guccio163/MoneyRequestReportPreview
Please assign this PR to me for review |
Hi, this PR is ready for the first reviews, but keep in mind there are still couple of aspects to work on:
These are the main issues to take care for now, potentially can be handled in follow-ups. Important: This component isn't incorporated in the app yet, the main place you can see it is |
@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] |
I think @allgandalf is going to take this while you work on reviewing the others, @DylanDylann. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
Very small nitpick we can fix as a follow up: it would be great if these buttons changed states at the same time... the slight delay between one going disabled and the other being disabled feels like a weird cascading effect. I would love for them to happen at the same time. CleanShot.2025-04-01.at.10.13.16.mp4 |
@shawnborton it's the same bug as described here for split expenses |
Got it - I assume we'll address in a follow up? |
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.
@luacmartins all yours
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.22-0 🚀
|
@Guccio163 Testers are not being able to open the expense details by clicking on the expense preview. Is this intended or known? Or should we create a new issue about this? Bug6789008_1743521688232.Recording__5315.mp4 |
It's known, coming in a follow-up. Here's a condensed list of things we're aware of and coming in a follow-up. |
🚀 Deployed to production by https://github.com/grgia in version: 9.1.22-10 🚀
|
Hi @Guccio163 @allgandalf @luacmartins @mountiny, I can't access the Design doc—could you help confirm if the format of |
@hoangzinh I shared the doc with you. I don't think we have that detail in the doc though. AFAIK the report name should follow the formula stored in |
Thank you @luacmartins |
data={transactions.slice(0, 11)} | ||
ref={carouselRef} | ||
nestedScrollEnabled | ||
scrollEnabled={transactions.length > 1} |
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.
Having scrollEnabled
false when transactions.length <= 1 led to this issue:
because having scrollEnabled false
was capturing touch on the ReportPreview, which being false
means it would block chat vertical scrolling.
We fixed the issue by removing the scrollEnabled
(making it true
by default), and instead added bounces={false}
to deal with a iOS: Native specific rubber band effect of scrollEnabled
being always true
.
Explanation of Change
This PR introduces
MoneyRequestReportPreview
- essentially designed to replaceReportPreview
(at least in most scenarios). Since some parts of logic are the same as inReportPreview
, there are parts of code already proven to work - there is a comment left by me to help with navigating that.This isn't a final product, since there are still couple of issues to work over, but can be handled in follow-ups - this PR is already quite big. Some of the things that are still to be done:
MoneyRequestReportView
on pressing the component.Fixed Issues
$ #57507
PROPOSAL:
Tests
Click right/left arrows to navigate between transactions.
b) Narrow screen (dots below transactions carousel visible)
Click on the dots to navigate to exact transactions.
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.mov
Android: mWeb Chrome
android-mWeb.mov
iOS: Native
Screen.Recording.2025-03-31.at.14.50.53.mov
iOS: mWeb Safari
Screen.Recording.2025-03-31.at.14.54.35.mov
MacOS: Chrome / Safari
Screen.Recording.2025-03-31.at.14.43.54.mov
MacOS: Desktop
Screen.Recording.2025-03-31.at.15.22.53.mov