-
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
Transactions - Update Generic Receipts to use ReceiptBackground #34843
Transactions - Update Generic Receipts to use ReceiptBackground #34843
Conversation
Hi @FitseTLT, could you complete the checklist? Thanks! For the tests, we should get step by step on each case to verify the issue is resolved. |
@mollfpr Completed! Good to go! |
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.
There are several issues I found.
Screen.Recording.2024-01-29.at.16.36.04.mp4
- The banner position is not center
- The thumbnail background color is changed after submit the money request
- The thumbnail banner size and position constantly change while navigating the page back and forth (on timeframe 0:18 - 0:23)
- The file extension is gone after the request is submitted to the API (on timeframe 0:23 - 0:25)
It's just random value 😅
This looks good to me! @FitseTLT Could you try to implement this? |
on it |
@shawnborton Updated. Here is how it looks |
I think that feels more consistent at least. Does anything happen if you tap on the receipt thumbnail area? I am guessing no right? |
Yeah! Nothing. |
@dangrous I think we are good to go here 👍 |
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.
This is looking good! Two quick questions and then I think I can get this through to final merging!
Great catch @dangrous . It would be nice to not have that label interactable |
@dangrous Done!! |
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.
Okay this looks good! @mollfpr can you give the new changes one last look please, and then we can get this going? Thank you both for such hard work!
@mollfpr bump on ^ |
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 and tests well 👍
Okay! Merging - thank you again! |
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.
not sure why this says I didn't review it, so just calling out that I did and trying again?
Details
Fixed Issues
$ #28827
PROPOSAL: #28827 (comment)
Tests
Offline tests
same
QA Steps
same
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.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 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
MacOS: Desktop