Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

Card - In issue card page, no outline box is shown around amount #52370

Open IuliiaHerets opened 3 days ago

IuliiaHerets commented 3 days ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com 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?

Screenshots/Videos

https://github.com/user-attachments/assets/18b77516-6907-42af-af99-f98dede98bbd

Bug6662261_1731403096396!Screenshot_2024-11-12-14-37-46-815_com android chrome

View all open jobs on GitHub

melvin-bot[bot] commented 3 days ago

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

melvin-bot[bot] commented 3 days ago

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[bot] commented 3 days ago

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

github-actions[bot] commented 3 days ago

:wave: 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 commented 3 days ago

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 commented 3 days ago

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

nkdengineer commented 3 days ago

I'm here.

shawnborton commented 3 days ago

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 commented 3 days ago

I think single-digit million might be realistic

dannymcclain commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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

dubielzyk-expensify commented 2 days ago

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: CleanShot 2024-11-13 at 14 01 05@2x

shawnborton commented 2 days ago

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 commented 2 days ago
Screenshot 2024-11-13 at 15 27 02

I see we require positive integers when we enter limit card

should we remove .00 in here @shawnborton @mountiny

Screenshot 2024-11-13 at 15 27 43
shawnborton commented 2 days ago

Hmm I don't feel too strongly really, I could go either way. Let's see it without so we can compare? I have a feeling we might like it better as it might feel way cleaner.

nkdengineer commented 2 days ago
Screenshot 2024-11-13 at 16 06 20

@shawnborton we have the result here

shawnborton commented 2 days ago

Cool, let's see what @mountiny and @Expensify/design think as well.

shawnborton commented 2 days ago

I might argue that the most consistent thing to do is always show the decimal places since that's what we do in places like Search.

nkdengineer commented 2 days ago
Screenshot 2024-11-13 at 16 44 02

This is the final result after I reduced the gap between columns, set the max limit value to 7 digits and fixed this bug

@shawnborton, @mountiny and @Expensify/design what do you think? cc @DylanDylann because you are C+ in the previous issue

shawnborton commented 2 days ago

Hmm, this isn't a real number: CleanShot 2024-11-13 at 10 29 12@2x

Can you take another look at that?

Also, what happens when all of the limits are smaller numbers?

DylanDylann commented 2 days ago

@mountiny Could you please assign me to this issue?

nkdengineer commented 2 days ago

Hmm, this isn't a real number

@shawnborton this is my mistake, I updated the screenshot here

Also, what happens when all of the limits are smaller numbers?

The result:

Screenshot 2024-11-13 at 16 48 18
shawnborton commented 2 days ago

The result:

Cool, it would be amazing if that column could shrink a bit based on how wide the content is. Seems like we have extra space we could use so that we can show more of the card name ya know?

dubielzyk-expensify commented 2 days ago

I really don't care strongly here. I prefer the look of a limit being without decimals. Can see the argument the other way, but because it's not an expense I also think we can afford the "inconsistency". Really don't mind either way 😅 Keen to hear if @dannymcclain has any thoughts

dannymcclain commented 2 days ago

Dang y'all I could go either way too. On the one hand, I see Shawn's perspective about always including the .00 to make it consistent with how we display amount elsewhere. On the other hand, since card limit can only accept whole numbers, I can see an argument for not displaying the .00.

How do we decide? Just go with displaying the .00 because that's how we do it elsewhere and it's the most consistent? Probably 🤷‍♂️

shawnborton commented 2 days ago

That works for me, or we just make @mountiny decide 😏

sakluger commented 2 days ago

We need to keep the decimals at the top (Current balance, Remaining limit, cash back) since those values will often have decimals.

For the individual limits, I personally like removing the decimals. I think that most of the time, users will be giving round-number limits (like $100, $1,000, $4,000, etc). Adding decimals on round numbers hurts readibility more than it does for non-round numbers.

mountiny commented 2 days ago

@sakluger it is a remaining limit on the card so after they make some purchase it will most likely need decimals to be correct

mountiny commented 2 days ago

Seems like there is a confusion, if i am not mistaken that number shows current remaining limit on the card, not the whole limit @DylanDylann @nkdengineer can you confirm please

mountiny commented 2 days ago

Hmm i might be wrong

dannymcclain commented 2 days ago

@mountiny Let's definitely get clarity on that before moving forward. That detail is pretty important for this decision 😂

I thought it was overall limit, not remaining, but let's find out for sure together!

mountiny commented 2 days ago

it is the unapprovedExpenseLimit from the card in that column and we have the availableSpend for the cards too but that is showed on the card details ✅

So yeah, most likely there would be no decimal spots!

@nkdengineer can you work on that then so we do not show the limit with decimals there?

nkdengineer commented 1 day ago

I'll raise PR soon

nkdengineer commented 1 day ago

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