App icon indicating copy to clipboard operation
App copied to clipboard

Bank account label in Workspace > Workflows > Make or track payments is too small

Open m-natarajan opened this issue 1 year ago • 2 comments
trafficstars

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.62-2 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @shawnborton Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1712858899464959

Action Performed:

  1. Go to Workspace Settings > Workflows and enable the "Make or track payments" toggle

Expected Result:

When there is no bank account added, the label for "Bank account" should be 15px in size

Actual Result:

The label is too small and is 13px in size

Workaround:

unknonw

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • [ ] Android: Native
  • [ ] Android: mWeb Chrome
  • [ ] iOS: Native
  • [ ] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

CleanShot 2024-04-11 at 20 06 20@2x

https://github.com/Expensify/App/assets/38435837/db82020b-89e3-429b-bc5c-c4e3e4952f8a

View all open jobs on GitHub

m-natarajan avatar Apr 13 '24 13:04 m-natarajan

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] avatar Apr 13 '24 13:04 melvin-bot[bot]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Bank account label in Workspace > Workflows > Make or track payments is too small

What is the root cause of that problem?

When hasVBA is true we are using textLabelSupportingNormal which has fontSizeLabel: getValueUsingPixelRatio(13, 19) for font size. The hasVBA is true because we even if we have reimburser login so we need to check for bankDisplayName also. https://github.com/Expensify/App/blob/db9e01cc302df7f2b3dd53a99c1082404676aedb/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx#L180

https://github.com/Expensify/App/blob/db9e01cc302df7f2b3dd53a99c1082404676aedb/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx#L87

What changes do you think we should make in order to solve the problem?

We should also check for bankDisplayName for applying styles.

titleStyle={hasVBA && bankDisplayName ? styles.textLabelSupportingNormal : styles.textLabelSupportingEmptyValue}

What alternative solutions did you explore? (Optional)

Krishna2323 avatar Apr 13 '24 14:04 Krishna2323

Proposal

Please re-state the problem that we are trying to solve in this issue.

Incorrect font size for Connect Bank Account text.

What is the root cause of that problem?

Currently, when we click on connect bank account we set hasVBA to true as we set the field reimburser inside of ACHAccount to the current policy owner email, but if we navigate back without connecting the bank account, then even though the bank account is not connected, yet hasVBA would be set to true as we have set the value of reimbursed: https://github.com/Expensify/App/blob/db9e01cc302df7f2b3dd53a99c1082404676aedb/src/types/onyx/Policy.ts#L193-L200

Now, this will set the style of the text to styles.textLabelSupportingNormal which has 13px as font size, but the text is still connect bank account and hence wrong style would be applied over here

What changes do you think we should make in order to solve the problem?

We should add a extra boolean condition like we do when we check which one to display between Connect Bank Account and Bank Account in the same component we check the condition as follows for text, so we can apply the same condition for style as well:

https://github.com/Expensify/App/blob/db9e01cc302df7f2b3dd53a99c1082404676aedb/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx#L182-L186

Update :

https://github.com/Expensify/App/blob/db9e01cc302df7f2b3dd53a99c1082404676aedb/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx#L179-L180

To:

 <MenuItem
      titleStyle={ hasVBA && policy?.reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES ? styles.textLabelSupportingNormal : styles.textLabelSupportingEmptyValue}
                            

What alternative solutions did you explore? (Optional)

allgandalf avatar Apr 13 '24 18:04 allgandalf

This issue will fix here https://github.com/Expensify/App/issues/39947, it has the same root cause. The style condition is correct hasVBA ? 13 : 15 but the issue is the hasVBA declaration is not correct after disable and then enable the "Make or track payments" for the first time, and this will fix here https://github.com/Expensify/App/issues/39947.

Tested in last main.

https://github.com/Expensify/App/assets/41129870/07b630bb-1a79-4595-ba7d-921530c20cab

ahmedGaber93 avatar Apr 13 '24 19:04 ahmedGaber93

I can confirm that this issue will be fixed as part of the already open PR https://github.com/Expensify/App/pull/40182 from issue:

  • https://github.com/Expensify/App/issues/39947.

Specifically the following lines of code:

  1. Line 89: Fixed hasVBA check.
  2. Line 90: Added shouldShowBankAccount variable.
  3. Line 191: Passed titleStyle correctly based on shouldShowBankAccount.

ikevin127 avatar Apr 14 '24 22:04 ikevin127

Thanks @ikevin127 , in that case, let's close this 👍

slafortune avatar Apr 15 '24 16:04 slafortune