[HOLD for payment 2024-11-26] Card - In issue card page, no outline box is shown around amount

[IuliiaHerets]

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: v9.0.60-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y
Issue reported by: Applause Internal Team

Action Performed:

1. Go to https://staging.new.expensify.com/home
2. Go to workspace settings
3. Enable expensify card and add bank account
4. Add both physical and virtual card with big amount

Expected Result:

In issue card page, outline box must be shown around amount.

Actual Result:

In issue card page, no outline box is shown around amount.

Workaround:

Unknown

Platforms:

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

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

bandicam.2024-11-12.11-41-47-727.mp4

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @sakluger

[melvin-bot]

Triggered auto assignment to @carlosmiceli (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[melvin-bot]

Triggered auto assignment to @sakluger (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]

💬 A slack conversation has been started in #expensify-open-source

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[mountiny]

Taking over as its card related. This is related to the new type column cc @nkdengineer @JmillsExpensify @Expensify/design

I think we need to make sure the amount is not trimmed

[mountiny]

@nkdengineer can you please comment as this is a regression from your PR so I think we should handle it together

[nkdengineer]

I'm here.

[shawnborton]

I agree with that, though do we have any Expensify cards that have a limit with that many digits? If so, let's fix to support it. But if not, I wonder if this is worth fixing or not.

[mountiny]

I think single-digit million might be realistic

[dannymcclain]

Agree that the limit should not be truncated, but just to be clear: the design is correct otherwise. We intentionally removed the badge treatment for limit during the updates to this table.

[shawnborton]

Yup, definitely agree with that. And yeah @mountiny sounds good, let's make sure we support that size. Ideally we could do it in a way that doesn't fix the column width (since most of these values won't be as wide as 9 digits) but rather in a way that allows the width to grow/shrink as needed.

[mountiny]

@nkdengineer can you please look into the options we have here to achieve such behaviour? thanks!

[dubielzyk-expensify]

Sorry for the nitpick, but is the spacing between the lines the same? And the line height? Feels like there's some horizontals not lining up:

[shawnborton]

Oh that's a good catch 🦅 👁️ agree that ideally those all line up perfectly. We can achieve that by making sure the name/login has the same line height as the limit, and then making sure the gap between the top and bottom texts are equal as well.

[nkdengineer]

@DylanDylann @mountiny @shawnborton @Expensify/design we have a open PR here

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.63-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix workspace card list style #52529

If no regressions arise, payment will be issued on 2024-11-26. 🎊

For reference, here are some details about the assignees on this issue:

• @DylanDylann requires payment (Needs manual offer from BZ)
• @nkdengineer requires payment (Needs manual offer from BZ)

[melvin-bot]

@DylanDylann @sakluger @DylanDylann The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[DylanDylann]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production
• 2b. Reported on staging (deploy blocker)
• 2c. Reported on both staging and production
• 2d. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

  Link to comment: NA. This is a new design

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

  Link to discussion:

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue:

Regression Test Proposal

Test:

1. Go to workspace with Expensify card enabled
2. Create a card
3. Verify that the limit amount isn't truncated when entering 8 or 9 numbers

Do we agree 👍 or 👎

[sakluger]

@mountiny can you please clarify if this is a regression and who needs payment? You mentioned in this comment that it was a regression from a previous PR, but other comments state that it was an oversight in the original design. Thanks!

[DylanDylann]

@sakluger I believe that it isn't regression. This seems like a new improvement. Thus, I think both me and @nkdengineer should get paid here

[mountiny]

I think this was a regression, we missed this case in the previous styles PR so I would not issue a payment here as the original issue should be paid out.

the original issue should be paid out at full though. No payments here

[sakluger]

Okay, thanks @mountiny for confirming! We're good to close 👍

[nkdengineer]

@sakluger @mountiny I agree with @DylanDylann that it isn't regression. The bug described in OP as expected has been confirmed here, and we are currently handling another bug where the limit is being truncated. And in the issue and PR, we also made quite a few style changes.

[mountiny]

@nkdengineer @DylanDylann I dont think so, I think this issue was a direct consequence of the previous PR and the fact we did not notice it in the previous design rounds or PR review is not and excuse

There its a third PR issue here #52663 that was also linked to these changes, I dont think it would be fair to issue $750 reward for all these changes that technically should have been done in one PR. So I think having $500 in total is more than enough in this case

[nkdengineer]

@mountiny The truncated bug that we fixed on this issue also happened before we merged the first PR and it also isn't mentioned in the first issue

Another point, I get paid 125$ on the first issue only

[mountiny]

So its $375 total as this was a regression #52663 from the PRs which leads to reduction of the original pay. So even if we consider this one as full one, I think it checks out. $250 for this one, $125 for the original one, because there was a regression in the design #52663 so $375 sounds right to me

[nkdengineer]

Thanks @mountiny, i agree with you 375$ for all. This issue is not paid yet so you mean we need to make a payment here, right?

[mountiny]

we can handle the payment #52663