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.

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

[HOLD][$250] AmEx icon is smaller than other card icons in the Filters #56567

Open
1 of 8 tasks
m-natarajan opened this issue Feb 8, 2025 · 41 comments
Open
1 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Design External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Feb 8, 2025

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:
Reproducible in staging?: Needs Reproduction
Reproducible in production?: Needs Reproduction
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
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 (hyperlinked to channel name): Expensify Bugs

Action Performed:

Prerequisite: Amex cards added as company cards

  1. Navigate to Report Search
  2. Click on filter and choose Card view

Expected Result:

All the cards displayed to select without any issues

Actual Result:

AmEx icon is smaller than other card icons in the Filters

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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

Add any screenshot/video evidence

Image

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021888023998366911450
  • Upwork Job ID: 1888023998366911450
  • Last Price Increase: 2025-02-08
  • Automatic offers:
    • ikevin127 | Contributor | 106169130
Issue OwnerCurrent Issue Owner: @sobitneupane
@m-natarajan m-natarajan added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Feb 8, 2025
Copy link

melvin-bot bot commented Feb 8, 2025

Triggered auto assignment to @NicMendonca (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.

@NicMendonca NicMendonca added External Added to denote the issue can be worked on by a contributor Design labels Feb 8, 2025
@melvin-bot melvin-bot bot changed the title AmEx icon is smaller than other card icons in the Filters [$250] AmEx icon is smaller than other card icons in the Filters Feb 8, 2025
Copy link

melvin-bot bot commented Feb 8, 2025

Job added to Upwork: https://www.upwork.com/jobs/~021888023998366911450

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 8, 2025
Copy link

melvin-bot bot commented Feb 8, 2025

Triggered auto assignment to @shawnborton (Design), see these Stack Overflow questions for more details.

Copy link

melvin-bot bot commented Feb 8, 2025

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

@shawnborton
Copy link
Contributor

I believe we already have the correct icon in the repo we should be using:

Image

@Shahidullah-Muffakir
Copy link
Contributor

Proposal

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

amex icon is smaller than other card icons in the Filters

What is the root cause of that problem?

  1. All the cards images used here are large:

    App/src/libs/CardUtils.ts

    Lines 262 to 274 in b0b7709

    const feedIcons = {
    [CONST.COMPANY_CARD.FEED_BANK_NAME.VISA]: Illustrations.VisaCompanyCardDetailLarge,
    [CONST.COMPANY_CARD.FEED_BANK_NAME.AMEX]: Illustrations.AmexCardCompanyCardDetailLarge,
    [CONST.COMPANY_CARD.FEED_BANK_NAME.MASTER_CARD]: Illustrations.MasterCardCompanyCardDetailLarge,
    [CONST.COMPANY_CARD.FEED_BANK_NAME.AMEX_DIRECT]: Illustrations.AmexCardCompanyCardDetailLarge,
    [CONST.COMPANY_CARD.FEED_BANK_NAME.BANK_OF_AMERICA]: Illustrations.BankOfAmericaCompanyCardDetailLarge,
    [CONST.COMPANY_CARD.FEED_BANK_NAME.CAPITAL_ONE]: Illustrations.CapitalOneCompanyCardDetailLarge,
    [CONST.COMPANY_CARD.FEED_BANK_NAME.CHASE]: Illustrations.ChaseCompanyCardDetailLarge,
    [CONST.COMPANY_CARD.FEED_BANK_NAME.CITIBANK]: Illustrations.CitibankCompanyCardDetailLarge,
    [CONST.COMPANY_CARD.FEED_BANK_NAME.WELLS_FARGO]: Illustrations.WellsFargoCompanyCardDetailLarge,
    [CONST.COMPANY_CARD.FEED_BANK_NAME.BREX]: Illustrations.BrexCompanyCardDetailLarge,
    [CONST.COMPANY_CARD.FEED_BANK_NAME.STRIPE]: Illustrations.StripeCompanyCardDetailLarge,
    [CONST.EXPENSIFY_CARD.BANK]: ExpensifyCardImage,
  2. but if the exact bank name is not found there, then we are returning small amex card image here:
    return Illustrations.AmexCompanyCards;

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

we can return amex large card image here:

return Illustrations.AmexCompanyCards;

as:

    return Illustrations.AmexCardCompanyCardDetailLarge;

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

NA

What alternative solutions did you explore? (Optional)

@DylanDylann
Copy link
Contributor

DylanDylann commented Feb 8, 2025

@Shahidullah-Muffakir The dark point here is why we return Illustrations.AmexCompanyCards. It should match with above conditions

@ikevin127
Copy link
Contributor

ikevin127 commented Feb 9, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-02-17 20:35:03 UTC.

Proposal

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

AmEx icon is smaller than other card icons in the Filters.

What is the root cause of that problem?

TLDR: Seems like we have an edge case regarding adding AMEX Company cards to a workspace and the data returned by BE which causes FE to display the fallback smaller AMEX card illustration.

In this function we map card icons to specific FE held CONST strings returned by BE once a card is added.
For AMEX we currently have the following two:

  • gl1025 (CONST.COMPANY_CARD.FEED_BANK_NAME.AMEX)
  • oauth.americanexpressfdx.com (CONST.COMPANY_CARD.FEED_BANK_NAME.AMEX_DIRECT)

which if mapped correctly to BE returned values, will display the AmexCardCompanyCardDetailLarge card illustration.

The reason why we see the small AMEX card illustration is because at the end of the function, if none of the FE held feed bank name constants match with the added AMEX feed bank name returned by BE, the function will display the smaller AmexCompanyCards card illustration as a fallback.

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

🗒 Since I wasn't able to reproduce on my side in order to extract the unknown AMEX feed bank name , in order to fix this issue accordingly we need help from BE to identify the missing AMEX feed bank name returned by BE in the case of the OP issue's workspace, that way we can add the new feed bank name to the FE held CONST list and return the correct illustration.

Note

I added a personal AMEX card via Workspace > Company cards, which for me returned oauth.americanexpressfdx.com (CONST.COMPANY_CARD.FEED_BANK_NAME.AMEX_DIRECT) meaning I could not reproduce the issue.

🔄 Update from BE

Quoting vit from Slack thread:

👋 What I see and is probably causing these issues are:

  • cards_2267989_ccupload17 (and similar numbers) we will support the csv upload in future but should we add some special icon for it?
  • cards_2267989_capitalonecards - not sure how this was added but its from oldDot I think
  • cards_2267989_citibusiness
  • cards_2267989_csvstripe
  • cards_2267989_discover.com - we support that in OldDot
  • cards_2267989_vcfhsbc (and other vcf something without number)
    Overall just seems like there is bunch of testing feeds and feeds from OldDot with a bit different naming convention.

I think what we should do it:

  1. Add generic default card if no match made
  2. Take suffix part and use it as name - so if its cards_2267989_csvstripe and we do not match with anythign we support in App - we will show All csvstripe as the name. This way there will be at least something
  3. Ensure we also match the Visa without numbers (if we dont already)
  4. ?? Should we have some new icon for the ccupload options? As those might be more common so they would deserve their own icon maybe

Based on the latest information quoted above from the Slack thread, and as suggested before this update, we should apply suggestions from (1.) to (4.) depending on how the Slack conversation flows, by doing the following:

  1. In this function, add OD missing constants to the map with new illustrations for each case.
  2. For any other value that falls outside of the known map values, return new generic fallback illustration.

(all new illustrations will be provided by @Expensify/design based on Slack conversation)

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A.

@sobitneupane
Copy link
Contributor

Thanks for the proposal @Shahidullah-Muffakir and @ikevin127

Below are the FEED_BANK_NAME values used to map the card to illustrations. It looks like at least one of them is missing. If this is something managed in our backend and not dependent on a third party, let's add the missing mapping from the BE to the FE (though I'm not entirely sure). (suggested by @ikevin127 here)

App/src/CONST.ts

Lines 2885 to 2897 in 15d9f03

FEED_BANK_NAME: {
MASTER_CARD: 'cdf',
VISA: 'vcf',
AMEX: 'gl1025',
STRIPE: 'stripe',
CITIBANK: 'oauth.citibank.com',
CAPITAL_ONE: 'oauth.capitalone.com',
BANK_OF_AMERICA: 'oauth.bankofamerica.com',
CHASE: 'oauth.chase.com',
BREX: 'oauth.brex.com',
WELLS_FARGO: 'oauth.wellsfargo.com',
AMEX_DIRECT: 'oauth.americanexpressfdx.com',
},

In any case, if there is no specific reason to return Illustrations.AmexCompanyCards as the fallback value, let's use Illustrations.AmexCardCompanyCardDetailLarge as suggested by @Shahidullah-Muffakir here.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Feb 10, 2025

Triggered auto assignment to @deetergp, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 10, 2025
Copy link

melvin-bot bot commented Feb 10, 2025

📣 @Shahidullah-Muffakir You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

@ikevin127
Copy link
Contributor

ikevin127 commented Feb 10, 2025

Note

More context

We're actively discussing this on Slack in this thread where it looks like the consensus seems to be that we're looking for the missing AMEX FEED_BANK_NAME value which means that we're possibly looking at adding the missing value to the map instead of replacing the fallback illustration (or maybe doing both).

This is the PR which added the initial return Illustrations.AmexCompanyCards; logic, then in a later refactor of the function this PR added the FEED_BANK_NAME values mapping logic including the 2 AMEX values. This PR later changed the illustration sizes to Large because of this issue (similar to ours). None of these follow-up PRs touched the outdated return Illustrations.AmexCompanyCards; logic.

♻ Tagged you both in the Slack thread as well where I explained more on why the selected proposal seems redundant to me and suggested to [HOLD] until the other involved Expensify team members confirm that we want to go this way.

cc @deetergp @sobitneupane

@deetergp deetergp changed the title [$250] AmEx icon is smaller than other card icons in the Filters [HOLD][$250] AmEx icon is smaller than other card icons in the Filters Feb 10, 2025
@deetergp deetergp assigned mountiny and unassigned deetergp Feb 11, 2025
@DylanDylann
Copy link
Contributor

Proposal

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

AmEx icon is smaller than other card icons in the Filters

What is the root cause of that problem?

@ikevin127's proposal already explained the RCA exactly and I have the same opinion as him

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

I don't understand why we return the AMEX card icon as default. It will be so confusing to users. Let's design a default card icon and return it here

return Illustrations.AmexCompanyCards;

In addition, we still need to make clear why the BE returns weird cards that don't match any cards defined in FE (Follow on Slack)

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

NA

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

NA

Copy link
Contributor

⚠️ @DylanDylann Thanks for your proposal. Please update it to follow the proposal template, as proposals are only reviewed if they follow that format (note the mandatory sections).

Copy link

melvin-bot bot commented Feb 17, 2025

@shawnborton, @NicMendonca, @sobitneupane, @mountiny, @Shahidullah-Muffakir Eep! 4 days overdue now. Issues have feelings too...

Copy link

melvin-bot bot commented Feb 18, 2025

📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@mountiny
Copy link
Contributor

I will go with @ikevin127 proposal and their help during the Slack conversion showing proactive approach. Lets use the generic card as a fallback and wait for Design team if we want to use some specific card icon for the csv import

@ikevin127

This comment has been minimized.

@ikevin127
Copy link
Contributor

🔄 Draft PR #57064 is ready to be opened once we decide on the generic card as fallback, @Expensify/design please refer to this Slack discussion comment & below for more context on the issue, requirements and new illustration (if we're going to have one).

Currently we have this Expensify card illustration (see below) which we could use as generic fallback even though it's not quite generic because of the VISA part, but the decision is yours on whether we should have a new illustration.

To recap on the issue:

  • it's possible that a user could have other types of cards imported via CSV and other sources including OD for which we currently fallback on this AMEX illustration which is smaller and doesn't look good / make sense since the other cards are not actually AMEX (see below in Before)

🟢 Solution:

  • issue's BE root cause and FE solution was summarized in this Slack discussion comment and here's a before / after fix with a possible new generic card illustration (currently used Expensify card illustration mentioned above)
Before After
Image Image

@shawnborton
Copy link
Contributor

We already have a generic card icon designed for when we don't have a bank/card logo available (see bottom right image):

Image

Can you check and see if it's already in the repo?

@shawnborton
Copy link
Contributor

Do we have a way to distinguish when a feed was added via CSV? We might consider doing a specific card icon that uses a csv icon as well, but I don't feel as strongly.

Image

@ikevin127
Copy link
Contributor

Can you check and see if it's already in the repo?

@shawnborton Yes, we already have that one added in the repo including a Large size. Here's an updated before / after:

Before After After (selected)
Image Image Image

Let me know if we want to go with something even more specific for the CSV edge cases or we're good with this one as general edge case fallback, then I'll be able to open the PR 👍

@mountiny
Copy link
Contributor

@ikevin127 confirmed and the csv uploads are the ones with _ccupload<number> so we can distinguish them

@shawnborton
Copy link
Contributor

shawnborton commented Feb 19, 2025

Sounds good! I exported a generic csv icons here for you, feel free to rename the files to match the other names we already have in the system:

cc @Expensify/design for viz

@ikevin127 your screenshots look good to me so far though!

@shawnborton
Copy link
Contributor

shawnborton commented Feb 19, 2025

Actually @Expensify/design I noticed this card uses our theme's icon color for the BG - so I assume we should have different versions depending on if we have dark mode or light mode. @ikevin127 what do you recommend we do there? Do you want separate files or can you control the fill of these icons using our color variables?

@dannymcclain
Copy link
Contributor

I'm down for having separate generic and CSV specific icons. And regarding the color, definitely not opposed to updating that to use a different BG color. We could even just pick a color to use for light and dark mode (not theme specific)?

Just an idea!
Image

@shawnborton
Copy link
Contributor

I don't feel strongly, I probably lean towards a more generic gray to communicate the generic nature of these? Let's see what the Judge thinks!

@ikevin127
Copy link
Contributor

can you control the fill of these icons using our color variables?

TLDR: Could be done with some code gymnastics, not sure it's worth it though since we can export them already colored.

These card illustrations have 3 different fill colors, therefore controlling the color through fill is not as simple as for other examples like icons which only have 1 fill color.

Let's see what the Judge thinks!

Who's The Judge ? 🤔

I think we can go about this in 2 main ways:

  1. Only have 1 generic gray illustration as fallback for any cards which are not part of the current map, falling outside.
  2. Have 2 different illustrations as shown here:
  • 1 generic (orange bg) which we use as fallback for any cards which are not part of the current map, falling outside
  • 1 generic CSV (pink bg) which we only use for card feeds including _ccupload in their name as mentioned in here

Note

As for going with option (2) and keeping both illustrations gray, that doesn't make much sense to me since there only difference in the illustration is the very small icon - so in this case I'd rather go with option (1) if we want to keep things gray:

Image

What I did below in the colored examples is I extracted the fill colors from here and changed them in the svg file for the sake of example, but if we're to go with option (2) I'll need both illustrations exported again as colored.

Before After (Option 1) After (Option 2)
Image Image Image

Additionally, if we want to go with option (2) I'll need a decision on copy regarding how we want to name the generic CSV card feed, examples (see picture above) could be:

  • All CSV
  • All CSV Cards
  • All CSV Imported

@shawnborton
Copy link
Contributor

Can we just supply you with different illustrations for light mode vs dark mode? I feel like we've done that elsewhere. This way we can use our icon gray that is specific to each theme. Thoughts?

@ikevin127
Copy link
Contributor

ikevin127 commented Feb 20, 2025

Can we just supply you with different illustrations for light mode vs dark mode?

@shawnborton Yes, it's possible if we add new logic implementation in the dark / light theme files and instead of passing colors / styles we pass illustrations then expose the defaultTheme variable in the getCardFeedIcon function in order to return theme-based illustrations.

▶ Took the liberty to modify current generic svg in 2 different themed variants for the purpose of demonstrating theme based illustration implementation:

See video
themed-illustrations.mov

I guess this could be option 3 in addition to my previous comment.

I feel like we've done that elsewhere.

Mind giving me an example from either UI or codebase ?

I wasn't able to find one - all I can see in terms of illustrations being used is that they are mostly passed to <Icon> components directly as source and sometimes they have fill color as I explained in #56567 (comment) for those icons which have only 1 fill color.

Actually, we do already have theme based illustrations logic 👍

@dannymcclain
Copy link
Contributor

Can we just supply you with different illustrations for light mode vs dark mode? I feel like we've done that elsewhere. This way we can use our icon gray that is specific to each theme. Thoughts?

@shawnborton that'd give us something like this:
Image

I think this looks pretty good. I also explored using other colors for the cards (borders, highlight-background, etc) but they all bummer out with the different row states (default, hover, selected) so I think this is the best option.

@melvin-bot melvin-bot bot added the Overdue label Feb 20, 2025
@shawnborton
Copy link
Contributor

Nice, that would be my vote then - I say let's do it! I'm mostly away from laptop today so I'll try to get the new assets over to Kevin either tomorrow or Monday.

@dannymcclain
Copy link
Contributor

I gotchu. Let's try these:

generic-card-images-light-dark.zip

That ZIP includes images for:

  • generic dark
  • generic light
  • generic csv dark
  • generic csv light
  • generic dark large
  • generic light large
  • generic csv dark large
  • generic csv light large

@shawnborton
Copy link
Contributor

Dope, thank you!

@ikevin127
Copy link
Contributor

Thanks for the feedback, I'm working on the PR which will be opened soon 👍

I was wrong in my previous comment, turns out we do already have theme based illustrations logic which will make the implementation even easier.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Overdue Daily KSv2 labels Feb 20, 2025
@ikevin127
Copy link
Contributor

♻ Opened the PR #57064, but awaiting for a copy decision - in the meantime I asked for an adhoc build in order for Shawn to test the changes and see how it looks on his side with all the edge cases since he's the bug report author, then adjust based on the feedback.

@dubielzyk-expensify
Copy link
Contributor

I like the new card illustration a lot 👍

@shawnborton
Copy link
Contributor

Nice, PR is looking good to me too 👍

@dannymcclain
Copy link
Contributor

Same!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Design External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: No status
Development

No branches or pull requests