-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Feature/kuba nowakowski/add empty state to expense view #58795
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
Feature/kuba nowakowski/add empty state to expense view #58795
Conversation
src/components/MoneyReportHeader.tsx
Outdated
@@ -346,6 +346,9 @@ function MoneyReportHeader({policy, report: moneyRequestReport, transactionThrea | |||
if (hasScanningReceipt) { | |||
return {icon: getStatusIcon(Expensicons.ReceiptScan), description: translate('iou.receiptScanInProgressDescription')}; | |||
} | |||
if (isEmpty) { |
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'm not sure if this should be done this way or as the expected optimistic next step. I did it this way because it was easier to implement and didn't require changes in Onyx.
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, I'll give it a thought and get back tomorrow
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.
Looks nice but I left some minor comments
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.
few comments, please address these
src/components/MoneyReportHeader.tsx
Outdated
@@ -199,6 +199,7 @@ function MoneyReportHeader({policy, report: moneyRequestReport, transactionThrea | |||
const isAdmin = policy?.role === CONST.POLICY.ROLE.ADMIN; | |||
|
|||
const filteredTransactions = transactions?.filter((t) => t) ?? []; | |||
const isEmpty = filteredTransactions.length === 0; |
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.
areFilteredTransactionsEmpty
? wdyt?
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.
If there are no transactions, it's a good indicator that we should display the empty state. This is possible right after creating reports.
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.
Cool thanks !
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.
actually no @mountiny need your input here we use isEmpty
below:

It doesn't affect logic but the name doesn't ring a bell to me what is empty , should we change it or keep it as is
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.
hasNoTransactions
might be more explicit
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.
maybe following our guidelines to not negate variable names, it should be !hasTransactions
src/components/MoneyReportHeader.tsx
Outdated
@@ -346,6 +346,9 @@ function MoneyReportHeader({policy, report: moneyRequestReport, transactionThrea | |||
if (hasScanningReceipt) { | |||
return {icon: getStatusIcon(Expensicons.ReceiptScan), description: translate('iou.receiptScanInProgressDescription')}; | |||
} | |||
if (isEmpty) { |
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, I'll give it a thought and get back tomorrow
src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx
Outdated
Show resolved
Hide resolved
src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx
Outdated
Show resolved
Hide resolved
@@ -218,7 +212,8 @@ function FloatingActionButtonAndPopover({onHideCreateMenu, onShowCreateMenu, isT | |||
isCreateMenuActive && (!shouldUseNarrowLayout || isFocused), | |||
); | |||
|
|||
const groupPoliciesWithChatEnabled = useMemo(() => getGroupPaidPoliciesWithExpenseChatEnabled(allPolicies as OnyxCollection<OnyxTypes.Policy>), [allPolicies]); | |||
const paidGroupPolicies = useMemo(() => Object.values((allPolicies as OnyxCollection<OnyxTypes.Policy>) ?? {}).filter((policy) => isPaidGroupPolicy(policy)), [allPolicies]); |
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 think we have a util already present to check this one, can you use that?
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.
Which utility are you referring to? Could you point me to the specific one?
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.
Is this what you want:
Lines 436 to 438 in 79dcb22
function getOwnedPaidPolicies(policies: OnyxCollection<Policy> | null, currentUserAccountID: number): Policy[] { | |
return Object.values(policies ?? {}).filter((policy): policy is Policy => isPolicyOwner(policy, currentUserAccountID ?? CONST.DEFAULT_NUMBER_ID) && isPaidGroupPolicy(policy)); | |
} |
@sumo-slonik this branch has conflicts, can those be fixed please |
…xpense-View # Conflicts: # src/pages/home/sidebar/FloatingActionButtonAndPopover.tsx
@DylanDylann 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] |
One thing I'm not sure if we covered - there should be a way to delete the report directly from the details view, right? Not sure if we need to consider it in this or not, but just wanted to flag it cc @mountiny @trjExpensify |
Otherwise this is feeling really good to me, nice work! |
if you think it needs to be changed then I will look into it, if not I guess that means we can merge? |
Yeah, it's something we could easily do as a follow up so we don't need to block on it for merge. |
@shawnborton to confirm, alternative is we should not show create report option if no workspace if present Screen.Recording.2025-03-28.at.4.13.03.PM.mov |
Screen.Recording.2025-03-28.at.4.14.52.PM.mov |
Screen.Recording.2025-03-28.at.4.16.47.PM.mov |
Screen.Recording.2025-03-28.at.4.18.24.PM.mov |
Good catch, I agree we should be able to create a report from the workspace chat. |
this was agreed to be handled in future once the views are ready we will incorporate them to the Inbox page too |
I agree with @allgandalf comments, but I'm not sure they are related to the empty state itself, when it comes to creating reports and expensives it's probably a separate issue |
Fixed in this PR #58903 (comment)
Agreed, you should be able to do that offline. Can be taken care in a follow-up.
Yes you should, and opened in the Inbox when you take that action. I agree with Vit to handle that when the views are ready for the Inbox page.
|
So if I understand correctly, all the bugs can be handled as a follow-up in the next PRs? |
YESS!!!!, Gimmme 5 to approve this one |
yeeeeah 🚀 |
Could @allgandalf List them all so we can put it into one issue and then also let QA know to check that before creating a new ones? |
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.
Thanks for all the hard work in this one @sumo-slonik ❤️ , looks good to get this one shipped
Here's the list of bugs:
- There is option to
Create Report
in FAB when no workspaces are present - No create report option in workspace chat
- New expenses added from the report view page doesn't get updated in the table of the report view.
- No option to add expense if report is created offline
- Expense tab is highlighted when report is opened in the search page
@mountiny feel free to create the tracking issue
thanks for the review 🤗 |
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.
One comment left and added those follow ups here #59295
@@ -229,7 +235,7 @@ function buildNextStep(report: OnyxEntry<Report>, predictedNextStatus: ValueOf<t | |||
}; | |||
|
|||
// Scheduled submit enabled | |||
if (harvesting?.enabled && autoReportingFrequency !== CONST.POLICY.AUTO_REPORTING_FREQUENCIES.MANUAL) { | |||
if (harvesting?.enabled && autoReportingFrequency !== CONST.POLICY.AUTO_REPORTING_FREQUENCIES.MANUAL && isReportContainingTransactions) { |
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 should add test for NextStepsUtils for this - added here #59295
Since carlos already approved, and design has done a thorough review, I am going to merge this one |
✋ 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/mountiny in version: 9.1.21-0 🚀
|
🚀 Deployed to production by https://github.com/grgia in version: 9.1.21-3 🚀
|
Explanation of Change
The created PR adds an empty state for the new report view and fixes a bug related to displaying two reports in the workspace.
Fixed Issues
$ #57654
$ #58426
$ #58425
$
PROPOSAL:
Tests
Offline tests
unnecessary
QA Steps
Same as test
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-03-20.at.14.45.18.mov
Android: mWeb Chrome
Screen.Recording.2025-03-20.at.14.51.28.mov
iOS: Native
Uploading Screen Recording 2025-03-20 at 15.51.45.mov…
iOS: mWeb Safari
Screen.Recording.2025-03-20.at.14.51.28.mov
MacOS: Chrome / Safari
Screen.Recording.2025-03-20.at.14.07.55.mov
MacOS: Desktop
Screen.Recording.2025-03-20.at.14.07.55.mov