-
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
[Details Revamp] Update ReportDetailsPage for Rooms, Groups, Money Reports and Policy Expenses #43251
[Details Revamp] Update ReportDetailsPage for Rooms, Groups, Money Reports and Policy Expenses #43251
Conversation
This comment has been minimized.
This comment has been minimized.
merge main into report-details-for-rooms-grous
As an update, during the creation of test steps I've encountered a case that isn't yet handled. I'm currently adding it into the page after which this PR will be able to be moved to review. |
…ls.getOriginalMessage
@grgia I've added the You could retest whether after the expense deletion you get redirected to the correct page, I've tested it myself and it should be good. After that it is ready to be merged. |
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.
Code changes LGTM
Triggering a test build to QA |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
Tests well! Looks great. I'm going to go ahead and |
✋ 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.1-0 🚀
|
@grgia @ikevin127 It would be great if you could help with the following about this PR.
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.1-19 🚀
|
!ReportUtils.isClosedReport(report) && | ||
canModifyTask; | ||
const canDeleteRequest = | ||
isActionOwner && (ReportUtils.canAddOrDeleteTransactions(moneyRequestReport) || ReportUtils.isTrackExpenseReport(transactionThreadReport)) && !isDeletedParentAction; |
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.
Here, we are checking ReportUtils.canAddOrDeleteTransactions(moneyRequestReport)
and ReportUtils.isTrackExpenseReport(transactionThreadReport)
to determine if we should show the delete request button.
This fails for self-DM track because, for self-DM track, transactionThreadReport is undefined, which causes canAddOrDeleteTransactions to return false. This caused this Issue #44334.
We have handled the issue by adding a check for isTrackExpenseReport and isSelfDM for canDeleteRequest value. More details can be found in the selected proposal #44334 (comment)
shouldShowRightIcon={!shouldDisableRename} | ||
interactive={!shouldDisableRename} | ||
title={reportName} | ||
titleStyle={styles.newKansasLarge} |
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.
Coming from #45346, If the report name cannot be edited, we should center it. We fixed that by adding titleContainerStyle
prop with styles.alignItemsCenter
style to MenuItemWithTopDescription
in nameSectionGroupWorkspace
.
}, [caseID, parentReport, report]); | ||
|
||
const canModifyTask = Task.canModifyTask(report, session?.accountID ?? -1); | ||
const shouldShowTaskDeleteButton = |
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.
We missed the canActionTask
check here, which caused #49408
onConfirm={deleteTransaction} | ||
onCancel={() => setIsDeleteModalVisible(false)} | ||
onModalHide={() => { |
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 is a race condition between deleting a transaction and navigating back. This results in brief moment where transaction page is still open but there is no data(showing Deleted transaction
. This is primarily noticeable in slower devices. #45576
Details
Update for the ReportDetailsPage for all the possible use-cases, such as Rooms and Groups, Money Reports/Requests and Policy Expense Report/Requests.
Fixed Issues
$ #42074
$ #42075
$ #42076
PROPOSAL:
Tests
Workspace Rooms & Groups:
IOU / Expense Reports:
Tasks:
Mark as incomplete
button which will cancel the completion status for the task.Here's a table which shows which PromotedActions should be displayed in the PromotedActionsBar for the report in each specific case.
Offline tests
QA Steps
Workspace Rooms & Groups:
IOU / Expense Reports:
Tasks:
Mark as incomplete
button which will cancel the completion status for the task.Refer to the PromotedActions table from Test steps to check whether every PromotedAction does properly display in each case specified in said table above.
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
Android.Native.mov
Android: mWeb Chrome
Android.Chrome.mov
iOS: Native
IOS.Native.mov
iOS: mWeb Safari
IOS.Safari.mov
MacOS: Chrome / Safari
Chrome.mov
MacOS: Desktop
Desktop.mov