Skip to content
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

Jump to bottom

Fix 25% opacity in MoneyRequestView and TaskView #27830

Merged
merged 2 commits into from
Sep 26, 2023
Merged

Fix 25% opacity in MoneyRequestView and TaskView #27830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1433235
Fix different grayness in MoneyRequestView and TaskView
ginsuma Sep 20, 2023
6f2fbfc
Clean code
ginsuma Sep 20, 2023
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
26 changes: 15 additions & 11 deletions src/components/ReportActionItem/MoneyRequestView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ function MoneyRequestView({report, parentReport, shouldShowHorizontalRule, trans
}

const isDistanceRequest = TransactionUtils.isDistanceRequest(transaction);
const pendingAction = lodashGet(transaction, 'pendingAction');
const getPendingFieldAction = (fieldPath) => lodashGet(transaction, fieldPath) || pendingAction;

return (
<View>
Expand All @@ -108,15 +110,17 @@ function MoneyRequestView({report, parentReport, shouldShowHorizontalRule, trans
</View>

{hasReceipt && (
<View style={styles.moneyRequestViewImage}>
<ReportActionItemImage
thumbnail={receiptURIs.thumbnail}
image={receiptURIs.image}
enablePreviewModal
/>
</View>
<OfflineWithFeedback pendingAction={pendingAction}>
<View style={styles.moneyRequestViewImage}>
<ReportActionItemImage
thumbnail={receiptURIs.thumbnail}
image={receiptURIs.image}
enablePreviewModal
/>
</View>
</OfflineWithFeedback>
)}
<OfflineWithFeedback pendingAction={lodashGet(transaction, 'pendingFields.amount') || lodashGet(transaction, 'pendingAction')}>
<OfflineWithFeedback pendingAction={getPendingFieldAction('pendingFields.amount')}>
<MenuItemWithTopDescription
title={formattedTransactionAmount ? formattedTransactionAmount.toString() : ''}
shouldShowTitleIcon={isSettled}
Expand All @@ -131,7 +135,7 @@ function MoneyRequestView({report, parentReport, shouldShowHorizontalRule, trans
subtitleTextStyle={styles.textLabelError}
/>
</OfflineWithFeedback>
<OfflineWithFeedback pendingAction={lodashGet(transaction, 'pendingFields.comment') || lodashGet(transaction, 'pendingAction')}>
<OfflineWithFeedback pendingAction={getPendingFieldAction('pendingFields.comment')}>
<MenuItemWithTopDescription
description={translate('common.description')}
shouldParseTitle
Expand All @@ -144,7 +148,7 @@ function MoneyRequestView({report, parentReport, shouldShowHorizontalRule, trans
numberOfLinesTitle={0}
/>
</OfflineWithFeedback>
<OfflineWithFeedback pendingAction={lodashGet(transaction, 'pendingFields.created') || lodashGet(transaction, 'pendingAction')}>
<OfflineWithFeedback pendingAction={getPendingFieldAction('pendingFields.created')}>
<MenuItemWithTopDescription
description={translate('common.date')}
title={transactionDate}
Expand All @@ -157,7 +161,7 @@ function MoneyRequestView({report, parentReport, shouldShowHorizontalRule, trans
subtitleTextStyle={styles.textLabelError}
/>
</OfflineWithFeedback>
<OfflineWithFeedback pendingAction={lodashGet(transaction, 'pendingFields.merchant') || lodashGet(transaction, 'pendingAction')}>
<OfflineWithFeedback pendingAction={getPendingFieldAction('pendingFields.merchant')}>
<MenuItemWithTopDescription
description={isDistanceRequest ? translate('common.distance') : translate('common.merchant')}
title={transactionMerchant}
Expand Down
20 changes: 8 additions & 12 deletions src/pages/home/report/ReportActionItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,12 +511,10 @@ function ReportActionItem(props) {
checkIfContextMenuActive: toggleContextMenuFromActiveReportAction,
}}
>
<OfflineWithFeedback pendingAction={props.action.pendingAction}>
<MoneyRequestView
report={props.report}
shouldShowHorizontalRule={!props.shouldHideThreadDividerLine}
/>
</OfflineWithFeedback>
<MoneyRequestView
report={props.report}
shouldShowHorizontalRule={!props.shouldHideThreadDividerLine}
/>
</ShowContextMenuContext.Provider>
);
}
Expand All @@ -538,12 +536,10 @@ function ReportActionItem(props) {
}

return (
<OfflineWithFeedback pendingAction={props.action.pendingAction}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you 100% confident that this is the same case? In the case of MoneyRequestView, you observed that the removed pending action props.action.pendingAction is equivalent to the existing conditions || lodashGet(transaction, 'pendingAction')}. I cannot see such a pattern in the case of TaskView.

I agree that it's great to fix another code fragment if it has an issue with the same root cause, but we're also responsible for all potential regressions we introduce.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can see the report.pendingFields and action.pendingAction both having CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD value when creating a task.

App/src/components/ReportActionItem/TaskView.js

Line 79 in 52dc94d

<OfflineWithFeedback pendingAction={lodashGet(props, 'report.pendingFields.reportName')}>

App/src/libs/actions/Task.js

Lines 94 to 99 in 52dc94d

pendingFields: {
createChat: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
reportName: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
description: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
managerID: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
},

App/src/libs/ReportUtils.js

Line 2388 in 52dc94d

pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it different from how it works in case of IOU actions?

What I'm trying to figure out is why the pattern of checking for pendingActions is different in TaskView and MoneyRequestView, i.e. that in MoneyRequestView we always check for either the outer pending action or for the field-level pending action, and in TaskView we only check for the field-level pending actions.

Copy link
Contributor Author

@ginsuma ginsuma Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I also see the different patterns between them. When creating,
in TaskView, we have pendingFields in the optimistic data. But in MoneyRequestView, we only have them when we update the fields. That's why we need to check || lodashGet(transaction, 'pendingAction') for add pending.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both IOU and Task actions have a similar set of operations, so I can't see a reason for these patterns to differ. But maybe we're exceeding the scope of this issue now...

Still, I think that if we're aware of these possible different combinations of pending actions (field-level vs the outer pending action), we should cover them in tests. Would you be able to describe the tests which ensure that the discussed || alternative works as expected after the PR changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

And, if possible, tests that affect tasks fields separately, demonstrating that such an alternative check is not needed, and we didn't break anything by removing the outer OfflineWithFeedback wrapper for tasks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests

Scenario 1:

  1. Be offline.
  2. Requesting money.
  3. Open the IOU details page.
  4. Verify that the grayness is as usual (50%).
  5. Be online.
  6. Verify that there is no grayness in the content.
  7. Be offline.
  8. Update Amount, Description, Date, Merchant.
  9. Verify that the grayness is as usual (50%).
  10. Be online.
  11. Verify that there is no grayness in the content.

Scenario 2:

  1. Be offline.
  2. Creating a task.
  3. Open the task details page.
  4. Verify that the grayness is as usual (50%).
  5. Be online.
  6. Verify that there is no grayness in the content.
  7. Be offline.
  8. Update Title, Description.
  9. Verify that the grayness is as usual (50%).
  10. Be online.
  11. Verify that there is no grayness in the content.

What do you think about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks fine 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the steps and videos.

<TaskView
report={props.report}
shouldShowHorizontalRule={!props.shouldHideThreadDividerLine}
/>
</OfflineWithFeedback>
<TaskView
report={props.report}
shouldShowHorizontalRule={!props.shouldHideThreadDividerLine}
/>
);
}
if (ReportUtils.isExpenseReport(props.report) || ReportUtils.isIOUReport(props.report)) {
Expand Down
Loading