Feature/kuba nowakowski/add empty state to expense view#58795
Conversation
src/components/MoneyReportHeader.tsx
Outdated
| if (hasScanningReceipt) { | ||
| return {icon: getStatusIcon(Expensicons.ReceiptScan), description: translate('iou.receiptScanInProgressDescription')}; | ||
| } | ||
| if (isEmpty) { |
There was a problem hiding this comment.
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.
hmm, I'll give it a thought and get back tomorrow
jnowakow
left a comment
There was a problem hiding this comment.
Looks nice but I left some minor comments
allgandalf
left a comment
There was a problem hiding this comment.
few comments, please address these
src/components/MoneyReportHeader.tsx
Outdated
| 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.
areFilteredTransactionsEmpty ? wdyt?
There was a problem hiding this comment.
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.
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.
hasNoTransactions might be more explicit
There was a problem hiding this comment.
maybe following our guidelines to not negate variable names, it should be !hasTransactions
src/components/MoneyReportHeader.tsx
Outdated
| if (hasScanningReceipt) { | ||
| return {icon: getStatusIcon(Expensicons.ReceiptScan), description: translate('iou.receiptScanInProgressDescription')}; | ||
| } | ||
| if (isEmpty) { |
There was a problem hiding this comment.
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
| ); | ||
|
|
||
| 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.
i think we have a util already present to check this one, can you use that?
There was a problem hiding this comment.
Which utility are you referring to? Could you point me to the specific one?
There was a problem hiding this comment.
Is this what you want:
Lines 436 to 438 in 79dcb22
|
@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? |
allgandalf
left a comment
There was a problem hiding this comment.
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 Reportin 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 🤗 |
|
|
||
| // 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.
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 Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.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