[Better Expense Report View] Remove reply count together with expense preview after deleting last expense #59679
Conversation
|
@allgandalf 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] |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-04-05.at.1.22.58.AM.movAndroid: mWeb ChromeScreen.Recording.2025-04-05.at.1.19.05.AM.moviOS: NativeScreenRecording_04-05-2025.01-15-03_1.MP4iOS: mWeb SafariScreen.Recording.2025-04-05.at.1.25.28.AM.movMacOS: Chrome / SafariScreen.Recording.2025-04-05.at.1.10.05.AM.movMacOS: DesktopScreen.Recording.2025-04-05.at.1.24.08.AM.mov |
|
🚧 @luacmartins has triggered a test app build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
| const useNewTableReportViewActionRenderConditionals = ({childMoneyRequestCount, childVisibleActionCount, pendingAction, actionName}: OnyxTypes.ReportAction) => { | ||
| const previousChildMoneyRequestCount = usePrevious(childMoneyRequestCount); | ||
|
|
||
| return !( | ||
| actionName === CONST.REPORT.ACTIONS.TYPE.REPORT_PREVIEW && | ||
| pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE && | ||
| childMoneyRequestCount === 0 && | ||
| (childVisibleActionCount ?? 0) > 0 && | ||
| (previousChildMoneyRequestCount ?? 0) > 0 | ||
| ); | ||
| }; |
There was a problem hiding this comment.
I think a method like this should be added to the utils file, and more docs should be added for better readability. Otherwise its harder for developers to follow what the method is for
There was a problem hiding this comment.
But that could be updated in a follow up
There was a problem hiding this comment.
+1 I found it confusing as someone who just looked at this for the first time. I would also say that it has a confusing name.
There was a problem hiding this comment.
Added it to the follow ups so we do not forget 🙌 #59452
|
✋ 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.24-2 🚀
|
| if (canUseTableReportView && isEmptyHTML(content)) { | ||
| return content; | ||
| if (canUseTableReportView && (isEmptyHTML(content) || (!shouldRenderViewBasedOnAction && !isClosedExpenseReportWithNoExpenses))) { | ||
| return emptyHTML; |
There was a problem hiding this comment.
I had some difficulty while working on this issue with money request previews not rendering appropriately and it led me to this change after some time of searching...
On dev, canUseTableReportView is true and the component is set to empty html here:
App/src/pages/home/report/PureReportActionItem.tsx
Lines 773 to 776 in 869e10d
So then we hit this point above and it won't render any of the selfDM's report previews after they are created.
Is it expected? @mountiny @luacmartins
There was a problem hiding this comment.
Ok I think maybe this feature just not fully built yet? And the beta is enabled on dev? 🫠
There was a problem hiding this comment.
Linked issue doesn't exist for me, but this conditional is changed in #59811 to be more precise and also other types of previews are handled by this PR, let's see after the merge.
And yes, this is under development, hence the beta flag 😄
App/src/pages/home/report/PureReportActionItem.tsx
Lines 778 to 799 in 43a2ffa
There was a problem hiding this comment.
Yeah sorry this is still behind beta in development, if its blocking you please disable the beta locally or use the PR Jakub shared above to get you unblocked
| const useNewTableReportViewActionRenderConditionals = ({childMoneyRequestCount, childVisibleActionCount, pendingAction, actionName}: OnyxTypes.ReportAction) => { | ||
| const previousChildMoneyRequestCount = usePrevious(childMoneyRequestCount); | ||
|
|
||
| return !( | ||
| actionName === CONST.REPORT.ACTIONS.TYPE.REPORT_PREVIEW && | ||
| pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE && | ||
| childMoneyRequestCount === 0 && | ||
| (childVisibleActionCount ?? 0) > 0 && | ||
| (previousChildMoneyRequestCount ?? 0) > 0 | ||
| ); | ||
| }; |
There was a problem hiding this comment.
+1 I found it confusing as someone who just looked at this for the first time. I would also say that it has a confusing name.
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.24-10 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.24-10 🚀
|
6 participants
Explanation of Change
Related to #59452
This fixes an issue where when deleting the last expense in the report, there is a delay in hiding the reply count compared to the report preview.
I'm using a custom hook here to separate the rest of the logic, because if the new view is properly linked, it will be easier to implement new logic in the right place and remove the old one.
Fixed Issues
$ #59465
PROPOSAL: N/A
Tests
Offline tests
Same as tests
QA Steps
Same as tests
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
Android.mov
Android: mWeb Chrome
iOS: Native
iOS.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Web.mov
MacOS: Desktop