-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
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: NativeScreen.Recording.2025-03-19.at.5.51.20.PM.movAndroid: mWeb ChromeScreen.Recording.2025-03-18.at.2.43.34.AM.moviOS: mWeb SafariScreen.Recording.2025-03-18.at.1.31.14.AM.movMacOS: DesktopScreen.Recording.2025-03-18.at.2.55.38.AM.mov |
@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. |
@ishpaul777 Fixed 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! 🧪🧪 |
cc @ishpaul777 for another review |
And cc @Expensify/design in case you want to take this for a spin and look for bugs - it's feeling pretty good to me though! Thanks so much @mkzie2 for being patient with our feedback. |
Actually @mkzie2 sorry, one small thing I noticed... it looks like the number portion of the eReceipt doesn't appear centered: In Figma, we're adding a space over on the right side of the amount so that it appears more centered for the majority of currency cases. Could we do something like that in the product? Basically it just adds like 16px of padding on the right side to balance out the currency symbol. Thanks! |
@shawnborton I added this. |
🚧 @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! 🧪🧪 |
@shawnborton Can you try to reload? ![]() |
I was trying in an incognito window and it still didn't work. Let me try again though... |
Okay cool, I'm seeing it now, nice: @ishpaul777 over to you for final review again, hopefully we can get this merged soon. Let's please do a super thorough test of this one, as I definitely want to avoid needing to revert :) |
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.
LGTM! Great work!
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.
Great work on this one everyone! It makes a big difference
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/grgia in version: 9.1.16-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.1.16-4 🚀
|
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