Skip to content

[Better Expense Report View] Remove reply count together with expense preview after deleting last expense #59679

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.

Sign up for GitHub

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

Merged
mountiny merged 2 commits into from
Apr 5, 2025
Merged

[Better Expense Report View] Remove reply count together with expense preview after deleting last expense #59679

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f7f78a9
Add hook to check if new table view should render action
JakubKorytko Apr 4, 2025
385c934
Merge branch 'main' into korytko/fix-reply-count-after-expense-removal
JakubKorytko Apr 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/pages/home/report/PureReportActionItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,18 @@ type PureReportActionItemProps = {
const emptyHTML = <RenderHTML html="" />;
const isEmptyHTML = <T extends React.JSX.Element>({props: {html}}: T): boolean => typeof html === 'string' && html.length === 0;

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
);
};
Comment on lines +331 to +341

@mountiny mountiny Apr 5, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mountiny mountiny Apr 5, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that could be updated in a follow up

@marcaaron marcaaron Apr 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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.

@mountiny mountiny Apr 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added it to the follow ups so we do not forget 🙌 #59452


/**
* This is a pure version of ReportActionItem, used in ReportActionList and Search result chat list items.
* Since the search result has a separate Onyx key under the 'snapshot_' prefix, we should not connect this component with Onyx.
Expand Down Expand Up @@ -390,7 +402,7 @@ function PureReportActionItem({
const [isEmojiPickerActive, setIsEmojiPickerActive] = useState<boolean | undefined>();
const [isPaymentMethodPopoverActive, setIsPaymentMethodPopoverActive] = useState<boolean | undefined>();
const {canUseTableReportView} = usePermissions();

const shouldRenderViewBasedOnAction = useNewTableReportViewActionRenderConditionals(action);
const [isHidden, setIsHidden] = useState(false);
const [moderationDecision, setModerationDecision] = useState<OnyxTypes.DecisionName>(CONST.MODERATION.MODERATOR_DECISION_APPROVED);
const reactionListRef = useContext(ReactionListContext);
Expand Down Expand Up @@ -1131,8 +1143,8 @@ function PureReportActionItem({
const renderReportActionItem = (hovered: boolean, isWhisper: boolean, hasErrors: boolean): React.JSX.Element => {
const content = renderItemContent(hovered || isContextMenuActive || isEmojiPickerActive, isWhisper, hasErrors);

if (canUseTableReportView && isEmptyHTML(content)) {
return content;
if (canUseTableReportView && (isEmptyHTML(content) || (!shouldRenderViewBasedOnAction && !isClosedExpenseReportWithNoExpenses))) {
return emptyHTML;

@marcaaron marcaaron Apr 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Table Report View does not display these components as separate messages
if (canUseTableReportView) {
children = emptyHTML;
}

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

@marcaaron marcaaron Apr 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I think maybe this feature just not fully built yet? And the beta is enabled on dev? 🫠

@JakubKorytko JakubKorytko Apr 9, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Table Report View does not display these components as separate messages, except for self-DM
if (canUseTableReportView && report?.type === CONST.REPORT.TYPE.CHAT) {
if (report.chatType === CONST.REPORT.CHAT_TYPE.SELF_DM) {
children = (
<View style={[styles.mt1, styles.w100]}>
<TransactionPreview
// eslint-disable-next-line @typescript-eslint/non-nullable-type-assertion-style
iouReportID={getIOUReportIDFromReportActionPreview(action) as string}
chatReportID={reportID}
reportID={reportID}
action={action}
isBillSplit={isSplitBillActionReportActionsUtils(action)}
wrapperStyles={shouldUseNarrowLayout ? {...styles.w100, ...styles.mw100} : reportPreviewStyles.transactionPreviewStyle}
onPreviewPressed={() => {}}
isTrackExpense={isTrackExpenseActionReportActionsUtils(action)}
/>
</View>
);
} else {
children = emptyHTML;
}
}

@mountiny mountiny Apr 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

if (draftMessage !== undefined) {
Expand Down