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 for payment 2024-09-06][$125] [Search v2.1] Update card filter to include the last four digits to the options #47324

Closed
luacmartins opened this issue Aug 13, 2024 · 30 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@luacmartins
Copy link
Contributor

luacmartins commented Aug 13, 2024

Coming from here, we should include the last four digits of the card in the options so that we know which card we're selecting when there are multiple options with the same name

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012701341a68cca511
  • Upwork Job ID: 1823710868220699994
  • Last Price Increase: 2024-08-14
  • Automatic offers:
    • ikevin127 | Reviewer | 103603179
Issue OwnerCurrent Issue Owner: @
Issue OwnerCurrent Issue Owner: @lschurr
@luacmartins luacmartins added Daily KSv2 NewFeature Something to build that is a new item. labels Aug 13, 2024
Copy link

melvin-bot bot commented Aug 13, 2024

Triggered auto assignment to @lschurr (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Aug 13, 2024
@luacmartins luacmartins added Daily KSv2 and removed Weekly KSv2 labels Aug 13, 2024
@daledah
Copy link
Contributor

daledah commented Aug 14, 2024

Edited by proposal-police: This proposal was edited at 2024-08-14 13:53:44 UTC.

Proposal

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

  • Update card filter to include the last four digits to the options

What is the root cause of that problem?

  • New feature.

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

  • We need to return additional data lastFourPAN: card.lastFourPAN in:

  • Update the CardListItem to display the lastFourPAN data:

    <View style={[styles.flexRow, styles.alignItemsCenter]}>
    <TextWithTooltip
    shouldShowTooltip={showTooltip}
    text={Str.removeSMSDomain(item.text ?? '')}
    style={[
    styles.optionDisplayName,
    isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText,
    item.isBold !== false && styles.sidebarLinkTextBold,
    styles.pre,
    item.alternateText ? styles.mb1 : null,
    ]}
    />
    </View>

                    <View style={[styles.flex1, styles.flexColumn, styles.justifyContentCenter, styles.alignItemsStretch, styles.optionRow]}>
                        <TextWithTooltip
                            shouldShowTooltip={showTooltip}
                            text={Str.removeSMSDomain(item.text ?? '')}
                            style={[
                                styles.optionDisplayName,
                                isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText,
                                item.isBold !== false && styles.sidebarLinkTextBold,
                                styles.pre,
                                item.alternateText ? styles.mb1 : null,
                            ]}
                        />
                        {!!item.lastFourPAN && (
                            <TextWithTooltip
                                shouldShowTooltip={showTooltip}
                                text={`Ending in ${item.lastFourPAN}`}
                                style={[styles.textLabelSupporting, styles.lh16, styles.pre]}
                            />
                        )}
                    </View>
              .map((card) => `${card.bank} (${card.lastFourPAN})`)

Demo

Screenshot 2024-08-20 at 03 33 06

What alternative solutions did you explore? (Optional)

  • Update the CardListItem to display the lastFourPAN data:
    <View style={[styles.flexRow, styles.alignItemsCenter]}>
    <TextWithTooltip
    shouldShowTooltip={showTooltip}
    text={Str.removeSMSDomain(item.text ?? '')}
    style={[
    styles.optionDisplayName,
    isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText,
    item.isBold !== false && styles.sidebarLinkTextBold,
    styles.pre,
    item.alternateText ? styles.mb1 : null,
    ]}
    />
    </View>
                <View style={[styles.flex1, styles.flexColumn, styles.justifyContentCenter, styles.alignItemsStretch, styles.optionRow]}>
                    <View style={[styles.flexRow, styles.alignItemsCenter]}>
                        <TextWithTooltip
                            shouldShowTooltip={showTooltip}
                            text={Str.removeSMSDomain(item.text ?? '')}
                            style={[
                                styles.optionDisplayName,
                                isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText,
                                item.isBold !== false && styles.sidebarLinkTextBold,
                                styles.pre,
                                item.alternateText ? styles.mb1 : null,
                            ]}
                        />
+                      <Text style={[styles.textAlignCenter, styles.textSupporting, styles.textNormal]}> • Ending in {item.lastFourPAN}</Text>
                    </View>
                </View>

Demo

image

@luacmartins luacmartins added the External Added to denote the issue can be worked on by a contributor label Aug 14, 2024
Copy link

melvin-bot bot commented Aug 14, 2024

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

@melvin-bot melvin-bot bot changed the title [Search v2.1] Update card filter to include the last four digits to the options [$250] [Search v2.1] Update card filter to include the last four digits to the options Aug 14, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 14, 2024
Copy link

melvin-bot bot commented Aug 14, 2024

Current assignee @ikevin127 is eligible for the External assigner, not assigning anyone new.

@luacmartins luacmartins changed the title [$250] [Search v2.1] Update card filter to include the last four digits to the options [$125] [Search v2.1] Update card filter to include the last four digits to the options Aug 14, 2024
Copy link

melvin-bot bot commented Aug 14, 2024

Upwork job price has been updated to $125

@luacmartins
Copy link
Contributor Author

@Expensify/design do we have a mock for how we wanna show the last 4 digits of the card?

@dannymcclain
Copy link
Contributor

I don't mind how it's shown in this proposal video, though since the user is likely looking for a specific card, I wonder if we should make it more prominent in the title.

@Expensify/design what do you all think?
CleanShot 2024-08-14 at 08 31 39@2x

Note

I grabbed a list with cards from Jon's card feed Figma file, if these icons and bank/card names are wrong, please forgive me. Just trying to get a sense of the type styles.

Also, will we have icons that match the cards? Or will we need something generic to sit on that left side?

@dubielzyk-expensify
Copy link
Contributor

Dig that mock though I kinda feel like we should have "Ending in 1414" or something. Might be repetative, but I feel that just the numbers below the card name feels a tad odd. Maybe even "···· 3128" or something

@shawnborton
Copy link
Contributor

Big fan of reusing the same card rows from the card feed project, and I think Jon's ideas for making the card number slightly more obvious are great too.

@dannymcclain
Copy link
Contributor

though I kinda feel like we should have "Ending in 1414" or something.

Yeah that makes perfect sense to me. I like Ending in #### because that's the way I typically see card numbers represented out in the wild wild web.

Dig that mock
Big fan of reusing the same card rows from the card feed project

So which style from here are you saying you all prefer? The left or the right? With the addition of the Ending in text, I think I prefer the right. Somethin' like this:

CleanShot 2024-08-15 at 08 37 10@2x

Alts just for fun:
CleanShot 2024-08-15 at 08 38 26@2x

@shawnborton
Copy link
Contributor

Nice! Yeah, the first mock you just shared is what I had in mind. I could get down with either of the alts too though. I guess you could argue the first mock is more consistent with existing selection list options though? Where we always use the subheading/description text on a second line?

@dannymcclain
Copy link
Contributor

Totally agree with you. I think the first option is the way to go!

@dubielzyk-expensify
Copy link
Contributor

Definitely agree that the first mock is the right fit! 👏 GG

@melvin-bot melvin-bot bot added the Overdue label Aug 16, 2024
@lschurr
Copy link
Contributor

lschurr commented Aug 19, 2024

What's the latest on this one? Is a contributor working on this or is it internal @luacmartins @ikevin127?

@melvin-bot melvin-bot bot removed the Overdue label Aug 19, 2024
@luacmartins
Copy link
Contributor Author

I think we're waiting on an updated proposal with the latest mocks

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 20, 2024
Copy link

melvin-bot bot commented Aug 20, 2024

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

Offer link
Upwork job

Copy link

melvin-bot bot commented Aug 20, 2024

📣 @daledah 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 📖

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 21, 2024
@ikevin127
Copy link
Contributor

ikevin127 commented Aug 21, 2024

❓ @Expensify/design I was reviewing the PR and noticed that the author did not add translations to the Ending in text yet. I checked out our current translations for similar / existing ones and I found this one which is not used anywhere:

cardEnding: ({cardNumber}) => `Card ending in ${cardNumber}`,

Question is: Can we use Card ending in 5555 or should we stick with the first mock from #47324 (comment) and add new & translate Ending in ?

Ending in Card ending in
Screenshot 2024-08-21 at 16 18 24 Screenshot 2024-08-21 at 16 16 49
[ES] Ending in [ES] Card ending in
Screenshot 2024-08-21 at 16 26 15 Screenshot 2024-08-21 at 16 20 06

@shawnborton
Copy link
Contributor

We want to use the new "Ending in..." and we should do that everywhere.

@ikevin127
Copy link
Contributor

⚠️ We just got confirmation on Slack that the Deploy Checklist: New Expensify 2024-08-26 which includes the PR of this issue was only deployed to production today in Deploy Checklist: New Expensify 2024-08-28. More context on why this happened can be found in this Slack thread and this Slack comment.

Given the context above, this issue should be on [HOLD for Payment 2024-09-6] according to today’s production deploy from Deploy Checklist: New Expensify 2024-08-28.

cc @lschurr @luacmartins

@luacmartins luacmartins changed the title [$125] [Search v2.1] Update card filter to include the last four digits to the options [HOLD for payment 2024-09-06][$125] [Search v2.1] Update card filter to include the last four digits to the options Sep 2, 2024
@luacmartins luacmartins added Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 and removed Reviewing Has a PR in review Weekly KSv2 labels Sep 2, 2024
@melvin-bot melvin-bot bot added the Overdue label Sep 2, 2024
@luacmartins
Copy link
Contributor Author

The PR solving this issue was deployed to prod in v9.0.26-6

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Sep 2, 2024
@lschurr
Copy link
Contributor

lschurr commented Sep 4, 2024

This is not overdue. Payment is Friday

@melvin-bot melvin-bot bot removed the Overdue label Sep 4, 2024
@lschurr
Copy link
Contributor

lschurr commented Sep 6, 2024

Payment summary:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Daily KSv2 labels Sep 8, 2024
@lschurr
Copy link
Contributor

lschurr commented Sep 9, 2024

Bump on this @daledah

@daledah
Copy link
Contributor

daledah commented Sep 10, 2024

can you provide your Upwork profile here

@lschurr Hey, it's https://www.upwork.com/freelancers/~0138d999529f34d33f

@lschurr
Copy link
Contributor

lschurr commented Sep 10, 2024

Thanks! Offer sent @daledah https://www.upwork.com/nx/wm/offer/103900230

@daledah
Copy link
Contributor

daledah commented Sep 10, 2024

@lschurr I accepted it thx

@lschurr lschurr closed this as completed Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
Status: Done
Development

No branches or pull requests

7 participants