Empty report chat view #59804
Merged
Empty report chat view #59804
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewContent.tsx # src/components/Search/index.tsx # src/components/SelectionList/Search/ReportListItem.tsx
8 tasks
|
Based on your video, it looks like the header area has too much padding: From Figma, the outer wrapper of the entire card should have 8px vertical padding. Then just the top header portion should have 6px vertical padding: This way that header area gets a height of 52px, which is a standard height we use for UI components. Same with the expense rows, this way they each are 52px tall and the 6px vertical padding gives us 12px between rows: |
|
I set the right and left padding to be the same as in the child (12 instead of 16 as in Figma). This is because we’re not using the new row component here yet — that will be done in a separate PR — and I wanted things to be aligned with the children. I explained more about it here. As for the top and bottom padding, I'm already working on the fixes. |
|
Sounds good, keep me posted! |
SzymczakJ
reviewed
Apr 9, 2025
src/components/ReportActionItem/MoneyRequestReportPreview/EmptyRequestReport.tsx
Outdated
Show resolved
Hide resolved
src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewContent.tsx
Outdated
Show resolved
Hide resolved
src/components/ReportActionItem/MoneyRequestReportPreview/EmptyRequestReport.tsx
Outdated
Show resolved
Hide resolved
Kicu
suggested changes
Apr 16, 2025
| </View> | ||
| <Button | ||
| success | ||
| text="PLACEHOLDER BUTTON" |
There was a problem hiding this comment.
probably we need to update this text?
There was a problem hiding this comment.
I don't think so — after merging the header buttons, there will be a primary button here, and this is just a placeholder that I'll remove later.
| import useTheme from '@hooks/useTheme'; | ||
| import useThemeStyles from '@hooks/useThemeStyles'; | ||
|
|
||
| function EmptyRequestReport() { |
There was a problem hiding this comment.
NAB but I wanted to propose a different name, reasons:
Reporthas some special meaning and just plain word report means usually a full-screen report view, or report-data.- we shouldn't use the name
RequestReportbecause there is no such thing. There isMoneyRequestReport, but if we cut out the "money" part then it gets confusing
My suggestions:
EmptyMoneyRequestReportPreviewMoneyRequestReportPreviewSkeletonEmptyReportPreview
or anything similar
There was a problem hiding this comment.
I'm already taking care of the changes
| <DelegateNoAccessModal | ||
| isNoDelegateAccessMenuVisible={isNoDelegateAccessMenuVisible} | ||
| onClose={() => setIsNoDelegateAccessMenuVisible(false)} | ||
| {transactions.length === 0 && <EmptyRequestReport />} |
There was a problem hiding this comment.
tiny nitpick, but since empty report preview is an important case, I suggest we extract the check to a const:
const shouldShowEmptyPlaceholder = transactions.length === 0
| import type * as OnyxTypes from '@src/types/onyx'; | ||
| import ActionCell from './ActionCell'; | ||
|
|
||
| type MoneyReportHeaderProps<TItem extends ListItem> = { |
There was a problem hiding this comment.
I think this name is a mistake?
Suggested change
| type MoneyReportHeaderProps<TItem extends ListItem> = { | |
| type ReportListItemHeaderProps<TItem extends ListItem> = { |
src/libs/Permissions.ts
Outdated
| @@ -51,7 +51,7 @@ function canUseCustomRules(betas: OnyxEntry<Beta[]>): boolean { | |||
| } | |||
|
|
|||
| function canUseTableReportView(betas: OnyxEntry<Beta[]>): boolean { | |||
| return !!betas?.includes(CONST.BETAS.TABLE_REPORT_VIEW) || canUseAllBetas(betas); | |||
| return true; | |||
| @@ -11,6 +11,10 @@ export default { | |||
| margin: 0, | |||
| }, | |||
|
|
|||
| m1: { | |||
There was a problem hiding this comment.
Do we have to add this value for literally only a single new case of margins? I think we can come up with something better :)
There was a problem hiding this comment.
I think this might be useful in the future as well. We don’t have m1:4 defined anywhere, but this kind of spacing scale does appear sometimes in mockups. If we never add it to our spacing system, we’ll always end up hardcoding it directly in styled components. It seems quite reasonable to include it in the spacing scale. We also discussed this with @SzymczakJ during his review.
| @@ -518,8 +518,7 @@ function getReportSections(data: OnyxTypes.SearchResults['data'], metadata: Onyx | |||
| } | |||
| } | |||
|
|
|||
| // Filter out reports with no transactions to prevent the wrong number of the selected options | |||
| return Object.values(reportIDToTransactions).filter((report) => report.transactions.length); | |||
| return Object.values(reportIDToTransactions); | |||
There was a problem hiding this comment.
Ahhh so many fond memories of many many hours spent catching these pesky reports without any transactions.
Remember @luacmartins? :D
I'm quite happy we're now making "empty" reports a first class citizen 👍
|
|
||
| type CellProps = { | ||
| showTooltip: boolean; | ||
|
|
|
|
||
| type ReportCellProps = { | ||
| reportItem: ReportListItemType; | ||
| } & CellProps; |
There was a problem hiding this comment.
I don't think you need both CellProps and ReportCellProps - this is a leftover, can be simplified
|
@sumo-slonik don't forget to clean up changes to |
|
I guess I'm a bit confused, but what's driving this detail in the header? It seems incorrect to me. Screenie above: Expected: |
|
I reused the component responsible for the header that appears in the report preview header. I'll fix it so that it looks like in the mockups. |
|
Let us know when we're ready for a test build, eager to start testing this one out! |
mountiny
changed the title
trjExpensify
changed the title
# Conflicts: # src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewContent.tsx # src/components/SelectionList/Search/ReportListItem.tsx
It seems to be working well with the new backend: I'm checking if nothing's broken and if everything's really working correctly, but at first glance it seems fine . |
As in, using the workspace's default report title for a newly created expense report? Looks like the avatar is correct now though, which was wrong in an earlier screenshot. 
|
|
Ready for a test build yet? |
I'm still working on the fixes for the report title — I should open the PR today and it should be ready for the test build.
Currently, I'm using the same logic for displaying avatars as in the report preview itself. I'm also adding some small changes to how the title is displayed. |
|
@sumo-slonik failed test |
1 task
|
Test failed after 360min, seems like a random CI bug or timeout and unrelated to code changes. |
|
For the breadcrumbs, I merged it coz' of trouble Carlos was having here: https://expensify.slack.com/archives/C07NMDKEFMH/p1746717739207839?thread_ts=1745948047.201979&cid=C07NMDKEFMH |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Dang Tommy's commit stats gonna go thru the roof now |
github-actions
bot
added
the
DeployBlockerCash
Performance Comparison Report 📊 (1/12)Significant Changes To Duration
Show details
|
Performance Comparison Report 📊 (10/12)Meaningless Changes To Duration (9/11)Show entries
Show details
|
Performance Comparison Report 📊 (11/12)Meaningless Changes To Duration (10/11)Show entries
Show details
|
Performance Comparison Report 📊 (12/12)Meaningless Changes To Duration (11/11)Show entries
Show details
|
Performance Comparison Report 📊 (2/12)Meaningless Changes To Duration (1/11)Show entries
Show details
|
Performance Comparison Report 📊 (3/12)Meaningless Changes To Duration (2/11)Show entries
Show details
|
Performance Comparison Report 📊 (4/12)Meaningless Changes To Duration (3/11)Show entries
Show details
|
Performance Comparison Report 📊 (5/12)Meaningless Changes To Duration (4/11)Show entries
Show details
|
Performance Comparison Report 📊 (6/12)Meaningless Changes To Duration (5/11)Show entries
Show details
|
Performance Comparison Report 📊 (7/12)Meaningless Changes To Duration (6/11)Show entries
Show details
|
Performance Comparison Report 📊 (8/12)Meaningless Changes To Duration (7/11)Show entries
Show details
|
Performance Comparison Report 📊 (9/12)Meaningless Changes To Duration (8/11)Show entries
Show details
|
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
@sumo-slonik I am getting policy expense chat in results. Can you please look into this? |
|
@shubham1206agra can you list the steps to reproduce, please? Thanks! 
|
|
Sure
|
mountiny
removed
the
DeployBlockerCash
|
Repro steps:
Main: Staging: |
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Fixed Issues
$ #59502
PROPOSAL:
Tests
Offline tests
Unnesesary
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-04-08.at.16.14.54.mov
Android: mWeb Chrome
mobile.android.mov
iOS: Native
Screen.Recording.2025-04-08.at.15.29.23.mov
iOS: mWeb Safari
ios.mobile.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
Uploading desktop.mov…