-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Improve eReceipt visually in New Dot #55803
base: main
Are you sure you want to change the base?
Conversation
@shawnborton @ishpaul777 One of you needs to 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] |
It seems like the spacing is quite off from our mockups. Here is what I see in your videos: Yet here is our Figma file: Can you update and then we can review again from there? |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
@mkzie2 can you please provide steps to add a card transaction (maybe we need to mock it? if yes please provide code to execute to have the mocked transaction) Thank you! |
Aside from @shawnborton's comments, why does the receipt change colors between the preview and full receipt? I would assume it would stay the same between those views. |
No problem, this is because I hard-coded it to display the receipt. |
@shawnborton, I updated it to match the Figma. Please help to check again. ![]() |
@ishpaul777 Updated the test step. |
dont we want make distance e-receipt to follow similar design @Expensify/design @mkzie2 |
I think we should leave the map receipts alone for now. We can follow up later and change them a bit to match these new eReceipts, but I say we tackle eReceipts first so that we don't have too much scope creep in this particular issue. |
It's 160px on my side in the ad-hoc build Screen.Recording.2025-02-19.at.21.28.05.mov |
On it now, I think I know the problem now, it's because we're not using the snap shot data for |
@shawnborton It's fixed now. |
🚧 @shawnborton has triggered a test 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! 🧪🧪
|
Totally agree with Shawn's comments. |
I think it's not an available solution in the front end. cc @grgia Can we generate eReceipt image for card transaction in back-end? Is this is a possible idea? |
Curious for your thoughts on this one @grgia when you get a minute, thanks! |
Why do you think it's not available on the front end? Basically instead of injecting all of this custom view/markup to create the eReceipt, you would do the same thing but using the SVG (xml) syntax. No idea if that would work on mobile though. |
@shawnborton I believe it's not possible because we depend on React-native using their provided Components to handle the rendering content in the App. We can not switch to using other syntax as per my understanding. maybe it would be possible to create a image link to receipt and push it from Backend like we do for per-diem receipt ![]() |
Yeah, I can see that being a good option. Let's see if @grgia has an opinion on that one, or maybe we need to bring this to Slack for discussion. |
@shawnborton If you can help me understand, whats your concern regarding rendering the receipt as is, is it because of the bugs, from rendering the receipt in a different places and sizes, or anything else? |
Yeah, basically in all other cases, the receipt preview is just an image. This means that all we have to do is give the receipt a width of 100% and it will fill whatever thumbnail container we want it to fill, whether that's a tiny thumbnail from the reports page, or a larger thumbnail when viewing the expense view. My concern here is that it feels like we need to rewrite eReceipt styles for every possible width that we might have, and it feels unsustainable and quite frankly a bit unpredictable. I would much rather just have a simple "image" of the eReceipt that can fill whatever space we need it to fill. Does that make sense? |
Basically this is the desired effect we want for eReceipts - it will just fill whatever container it is in. The same exact way we handle receipt images: CleanShot.2025-02-24.at.13.56.01.mp4 |
i agree with your point of having the component work flawlessly no matter where it it used 👍 , i think @mkzie2 has wrote some code logic to scale the the receipt according to the parent container width it just does not work properly, @mkzie2 do you think you would be able to refine the logic make the component work flawlessly as shown in video ^ or should we be exploring other solutions, Thank you for your hard work so far! |
I will investigate this solution again and give an update tomorrow |
@shawnborton, please help check if it's the correct result. result-1.mov![]() |
That does seem better, let's fire up a test build and take it for a spin! |
🚧 @shawnborton has triggered a test hybrid 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! 🧪🧪 |
Hmm that's an improvement but it's still not technically working exactly as I was showing above. It's not truly scaling at it's exact proportions depending on the outer container width. What can we do about that? Also, on the reports page, most of the eReceipts are yellow and don't have an icon: CleanShot.2025-02-26.at.15.07.29.mp4What's going on there? Because then when I open up the receipt, it shows the icon and it's even a different color. |
Explanation of Change
Improve eReceipt visually in New Dot
Fixed Issues
$ #55083
PROPOSAL: #55083 (comment)
Tests
_
with the transactionID that we're viewing)or add this
useEffect
toMoneyRequestView
Offline tests
Same
QA Steps
or add this
useEffect
toMoneyRequestView
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
Screen.Recording.2025-02-03.at.16.17.38.mov
Android: mWeb Chrome
Screen.Recording.2025-02-03.at.15.08.30.mov
iOS: Native
Screen.Recording.2025-02-03.at.16.23.20.mov
iOS: mWeb Safari
Screen.Recording.2025-02-03.at.15.10.11.mov
MacOS: Chrome / Safari
Screen.Recording.2025-02-03.at.15.04.40.mov
MacOS: Desktop
Screen.Recording.2025-02-03.at.16.26.39.mov