-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
fix: resolve dupe button on one-expense report #50133
base: main
Are you sure you want to change the base?
Conversation
@sobitneupane 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] |
@daledah The "Review Duplicate" button remains visible even after one of the duplicate expenses is deleted. Steps to Reproduce:
Screen.Recording.2024-10-04.at.11.49.44.mov |
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.
@dannymcclain could you please review the screenshots attached above?
|
@sobitneupane Can you detail your steps in #50133 (comment)? I can't seem to replicate the exact condition as in your video. |
@daledah I can no longer reproduce the issue. I’ll follow up if I manage to replicate it again. |
The screenshots look a bit off to me. I think the On mobile, the buttons should be next to each other instead of stacked. cc'ing @dubielzyk-expensify & @JmillsExpensify for a gut check on all this. |
That looks right to me Danny! |
@daledah Are all the buttons consistent with what @dannymcclain posted? |
@pecanoro I'll update soon. |
@dannymcclain @pecanoro I updated |
Updated screenshots/videos are looking good. On mobile, it looks like there's a second or two when the report is loading that the big green button (Approve for example) is full-width, and then the Resolve duplicates button flashes in and the layout changes—is there any way to get rid of that jump or is it just a consequence of how we're loading the state of the report? Otherwise layout is looking good to me. |
@sobitneupane Ready for you to do another review! |
|
@@ -247,6 +248,11 @@ function MoneyReportHeader({policy, report: moneyRequestReport, transactionThrea | |||
[chatReport?.isOwnPolicyExpenseChat, policy?.harvesting?.enabled], | |||
); | |||
|
|||
const shouldDuplicateButtonBeSuccess = useMemo( | |||
() => isDuplicate && !shouldShowSettlementButton && !shouldShowExportIntegrationButton && !shouldShowSubmitButton && !hasAllPendingRTERViolations, |
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 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.
@sobitneupane I think submit button not being success
-themed is expected:
App/src/components/MoneyReportHeader.tsx
Line 244 in 392d56a
// The submit button should be success green colour only if the user is submitter and the policy does not have Scheduled Submit turned 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.
I believe so. In that case, should we have Review duplicates button success
? cc: @dannymcclain
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.
🤔 Hmm, maybe? In this case I'm not sure. I don't think it's wrong to have two regular buttons here. But maybe @JmillsExpensify will have a better informed opinion on if the review duplicates button should be success
colored in this situation.
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.
It's not wrong to have two regular buttons. This is the case of delayed submission being enabled, with a submission frequency set.
@sobitneupane I updated, please review again. |
Details
Fixed Issues
$ #49793
PROPOSAL: #49793 (comment)
Tests
Precondition:
Steps:
Offline tests
QA Steps
Precondition:
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
Android: Native
Screen.Recording.2024-10-03.at.15.45.13.mov
Android: mWeb Chrome
Screen.Recording.2024-10-03.at.15.46.21.mov
iOS: Native
Screen.Recording.2024-10-03.at.15.49.31.mov
iOS: mWeb Safari
Screen.Recording.2024-10-03.at.15.50.31.mov
MacOS: Chrome / Safari
Screen.Recording.2024-10-03.at.15.12.26.mp4
MacOS: Desktop
Screen.Recording.2024-10-03.at.15.54.12.mov