-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[Follow-up fix] Correct color pick for EReceipt Thumbnails and remove duplication of MCC and Trip icons. #45331
[Follow-up fix] Correct color pick for EReceipt Thumbnails and remove duplication of MCC and Trip icons. #45331
Conversation
merge main into @cdOut/receipt-color-fix
@ 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] |
The PR is ready to be tested, I've reverted to the previous way of assigning colors and cleared up the conditionals to only ever show either the MCC Icon or the Trip Icon. |
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. Is there an App Issue for this one?
@grgia Don't think there's an issue created for this one. I've added a patch to fix the font scalling issue. The patch should be removed after updating |
cc: @shubham1206agra |
@cdOut Merge main please. |
merge main into @cdOut/receipt-color-fix
@shubham1206agra merged latest main. |
merge main into @cdOut/receipt-color-fix
Reassurance tests seem to be failing due to outdated node-modules which don't have the patch included in this PR applied to them, do you know a fix to this one @shubham1206agra ? |
@cdOut Maybe make a new patch instead of modifying old ones |
This reverts commit 130d400. revert modifying patch
merge main into @cdOut/receipt-color-fix
@shubham1206agra I've created a separate patch file and merged with latest main. |
Reviewer Checklist
Screenshots/VideosAndroid: Native |
🎯 @shubham1206agra, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #45747. |
cc: @grgia |
@cdOut performance test failing |
I am unsure on how to fix this one, I'm fairly sure I've created the new patch correctly. @shubham1206agra any ideas? |
@cdOut Could you try merging your branch with |
merge main into @cdOut/receipt-color-fix
@fabioh8010 merged newest main, we'll see if that resolves it once the gh actions are done |
All good, @grgia ready for your re-review. |
@shubham1206agra any comments before I merge? |
You can merge here |
✋ 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.0.12-0 🚀
|
@grgia @shubham1206agra we can't make e-receipt transactions or trip transaction. Should we QA this one? |
@twisterdotcom Does travel PR not come under QA? |
@grgia would you be able to internal-QA this one with an e-receipt please? Thanks! |
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.12-0 🚀
|
🚀 Deployed to staging by https://github.com/grgia in version: 9.0.13-0 🚀
|
Details
Revert to the old way of choosing EReceipt Thumbnail colors and add a conditional to check whether to display MCC or Trip icon in the preview. It also applies a patch for react-native to fix an issue with font scalling on IOS.
Patch applied here should be removed after updating
react-native
to version 0.75. This is being done in this PR.Fixed Issues
$ https://swmansion.slack.com/archives/C049HHMV9SM/p1720705558021579
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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
MacOS: Chrome / Safari