Bank Icons add for payment methods #4653

Santhosh-Sellavel · 2021-08-13T19:36:01Z

Details

Added bank icons for cards & bank account payment options

Fixed Issues

$ #4398

Tests & QA Steps

Go to Settings -> Payments
Your Card & Bank payment methods should be their respective bank icons. Or should show our default bank or card icons.

Note: Refer to screenshot & ensure default icons & Expensify are in small size & bank logos slightly bigger as shown.

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Santhosh-Sellavel · 2021-08-13T19:58:08Z

@stitesExpensify I see lint error will fix those asap, please ignore it for time being, thanks!

stitesExpensify

Looking good! What is the problem you mentioned with Fidelity and Citizens bank here? Can you give me an example?

assets/images/bankicons/discover.svg

src/components/Icon/BankIcons.js

Santhosh-Sellavel · 2021-08-17T19:54:51Z

Looking good! What is the problem you mentioned with Fidelity and Citizens bank [here]
(#4398 (comment))? Can you give me an example?

@stitesExpensify
For Fidelity, what banks to take from here. Look at the namings I don't know whether they are the same or different organizations

For Citizens bank,

Minified Mapping results for fidelity & Citizen's Bank have a look at it.

{"fidelity":[{"bankName":"FIDELITY BANK","assetName":"fidelity"},{"bankName":"The Fidelity Bank (NC) - Personal","assetName":"fidelity"},{"bankName":"Fidelity","assetName":"fidelity"},{"bankName":"FIDELITY CO-OP BANK","assetName":"fidelity"},{"bankName":"FIDELITY DEP DISCOUNT BK","assetName":"fidelity"},{"bankName":"THE FIDELITY BANK","assetName":"fidelity"},{"bankName":"FIDELITY BANK AND TRUST","assetName":"fidelity"},{"bankName":"FIDELITY NATIONAL BANK","assetName":"fidelity"},{"bankName":"FIRST FIDELITY BANK  N.A.","assetName":"fidelity"},{"bankName":"FIDELITY BANK OF TEXAS","assetName":"fidelity"},{"bankName":"First Fidelity Bank (OK & AZ)","assetName":"fidelity"},{"bankName":"FIDELITY FED SAVINGS & LOA","assetName":"fidelity"},{"bankName":"Fidelity Bank (LA)","assetName":"fidelity"},{"bankName":"Fidelity Bank (Georgia) - Business","assetName":"fidelity"},{"bankName":"Fidelity Bank (Georgia)","assetName":"fidelity"},{"bankName":"Fidelity Bank","assetName":"fidelity"},{"bankName":"First Fidelity Bank","assetName":"fidelity"},{"bankName":"Fidelity Charitable Gift Fund","assetName":"fidelity"},{"bankName":"Fidelity Bank (IA)","assetName":"fidelity"},{"bankName":"United Fidelity Bank - Personal","assetName":"fidelity"},{"bankName":"FIRST FIDELITY BANK","assetName":"fidelity"},{"bankName":"Fidelity Bank & Oklahoma Fidelity Bank - Personal Online Banking","assetName":"fidelity"},{"bankName":"Fidelity Bank (PA) - Personal","assetName":"fidelity"}],"citizens-bank":[{"bankName":"citizensbankbusiness","assetName":"citizens-bank"},{"bankName":"Citizens Bank","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK NA","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK  NATIONAL ASSOCIATION","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK OF LAFAYETTE","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK OF MUKWONAGO","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK & TRUST CO","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK OF PHILADELPHIA","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK OF EDMOND","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK & TRUST OF CHILLICOTHE","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK MINNESOTA","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK & TRUST CO.","assetName":"citizens-bank"},{"bankName":"Citizens Bank & Trust (MO)","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK AND TRUST CO","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK OF CHATSWORTH","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK AND TRUST","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK OF ADA","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK AND TRUST COMPANY","assetName":"citizens-bank"},{"bankName":"Citizens Bank Minnesota","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK & TRUST BIG TIMBER","assetName":"citizens-bank"},{"bankName":"Citizens Bank & Trust Company","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK OF FARMINGTON","assetName":"citizens-bank"},{"bankName":"Citizens Bank (Swainsboro, Laurens County)","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK OF KANSAS  N.A.","assetName":"citizens-bank"},{"bankName":"Citizens Bank of Lafayette","assetName":"citizens-bank"},{"bankName":"Citizens Bank & Trust","assetName":"citizens-bank"},{"bankName":"Citizens Bank of Mukwonago","assetName":"citizens-bank"},{"bankName":"Citizens Bank & Trust Company (Big Timber, Montana)","assetName":"citizens-bank"},{"bankName":"Citizens Bank & Trust Company (VA)","assetName":"citizens-bank"},{"bankName":"Citizens Bank & Trust (Alabama)","assetName":"citizens-bank"},{"bankName":"Citizens Bank of Edmond","assetName":"citizens-bank"},{"bankName":"Citizens Bank of Kansas","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK OF KENTUCKY, INC","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK, NATIONAL ASSOCIATION","assetName":"citizens-bank"},{"bankName":"Citizens Bank of Georgia","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK OF KANSAS","assetName":"citizens-bank"},{"bankName":"Citizens Bank & Trust (Florida)","assetName":"citizens-bank"},{"bankName":"Citizens Bank & Trust Co. (Vivian, Louisiana)","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK OF LAS CRUCES","assetName":"citizens-bank"},{"bankName":"CITIZENS BANK & TRUST COMPANY","assetName":"citizens-bank"}]}

Santhosh-Sellavel · 2021-08-17T20:01:07Z

Citizens Bank & Trust & Citizens Bank of * Is it the same bank or different?
Fidelity is a too generic name, there are different bank name variations. I don't whether they are the same bank or different.
See the names here give me some input what to do with this @stitesExpensify

stitesExpensify · 2021-08-17T20:55:01Z

Citizens Bank & Trust & Citizens Bank of * Is it the same bank or different?

Same bank

Fidelity is a too generic name, there are different bank name variations. I don't whether they are the same bank or different.

I think it's all the same.

Let's just assume that anything with "citizens bank" in it is citizens, and anything with "Fidelity" in it is fidelity. I don't think the false positives really matter if the bank is so obscure that it has one of those things in it

Santhosh-Sellavel · 2021-08-21T18:26:19Z

@stitesExpensify I don't have a real account with bank account linked.
So I need your help me with test with real time data.

I used some dummy data for to have a peek. Here is my some screenshots, is it okay do you think we need some additional styling? let me know.

Android

web

Thanks!

Santhosh-Sellavel · 2021-08-26T03:37:20Z

@stitesExpensify Just a reminder, I had done the requested changes. Let me know If there are any issues thanks!

Santhosh-Sellavel · 2021-08-27T18:20:54Z

@stitesExpensify Any update on this?

stitesExpensify · 2021-08-27T18:38:52Z

My apologies, still no update. I am completely swamped with internal priorities. I will get to it as soon as I can

Santhosh-Sellavel · 2021-08-27T18:43:27Z

Thanks for notifying me!

stitesExpensify · 2021-08-31T22:50:39Z

src/components/Icon/BankIcons.js

+/**
+ * Returns matching asset icon for bankName
+ * @param {String} bankName
+ * @returns


Need a return type

@stitesExpensify What's the return type here can you help me here.

I believe it should just be Object

Updated as Object

stitesExpensify · 2021-08-31T22:52:59Z

src/components/Icon/BankIcons.js

+
+export default function getBankIcon(bankName) {
+    if (!bankName) {
+        return DefaultBank;


So can we not get the default credit card icon anymore? We still want that for cards that don't match anything

We can do it since we are trying to get Icon for Bank Name by default it should be a Bank Logo.
I'll make the changes if needed, let me know about the default card icon size whether new size or the old size. Also confirm for default bank size also.

Default bank size looks good to me, I think card can be the same.

We definitely want a card icon when there are no matches.

I think adding a param to getBankIcon for isCard and returning the default card icon would be a good solution for this

stitesExpensify · 2021-08-31T22:54:17Z

#4653 (comment)

I don't think we need more styling, it looks good to me. What about you @shawnborton

shawnborton · 2021-09-01T08:33:51Z

Yup, I think it's good to go.

stitesExpensify · 2021-09-01T21:49:25Z

One more request please, can you add one more condition to bankIcons.js for the expensify card specifically?

if(bankName === 'Expensify Card') {
    return expensifyCard
}

expensifyCard is in assetes/images/expensifycard.svg

Sorry that was not in the original request. Thanks!

Santhosh-Sellavel · 2021-09-02T05:50:00Z

@stitesExpensify

I think we are good with this, then.

shawnborton · 2021-09-02T09:16:38Z

The first three icons here look way too large.

…

On Thu, Sep 2, 2021 at 7:50 AM Santhoshkumar Sellavel < ***@***.***> wrote: @stitesExpensify <https://github.com/stitesExpensify> I think we are good with this, then. [image: Screenshot 2021-09-02 at 11 18 51 AM] <https://user-images.githubusercontent.com/85645967/131789067-859006eb-2d43-41cd-8be7-0a9f4e56b49d.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4653 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARWH5XGKWRRBDQ4N4HDOLDT74GBHANCNFSM5CEFPQDQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

Santhosh-Sellavel · 2021-09-02T09:18:50Z

@shawnborton
Following the same size as the bankicon, as shown below.

shawnborton · 2021-09-02T09:21:17Z

It should be the same size that it was previously, it should match the PayPal icon. The bank logos look correct though.

…

On Thu, Sep 2, 2021 at 11:19 AM Santhoshkumar Sellavel < ***@***.***> wrote: @shawnborton <https://github.com/shawnborton> Following the same size as the Logo [image: 130331487-058b2c63-9550-4e04-84ab-08fd0cbe6700.png] <https://user-images.githubusercontent.com/85645967/131818029-212c8ede-b9e5-4cf2-9641-cc54ffddaf56.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4653 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARWH5VLJL2ULUOTSJZ45RDT746QLANCNFSM5CEFPQDQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

Santhosh-Sellavel · 2021-09-02T09:29:00Z

@shawnborton
How about this?

shawnborton · 2021-09-02T09:30:03Z

That looks good to me, thanks!

…

On Thu, Sep 2, 2021 at 11:29 AM Santhoshkumar Sellavel < ***@***.***> wrote: @shawnborton <https://github.com/shawnborton> How about this [image: Screenshot 2021-09-02 at 2 57 57 PM] <https://user-images.githubusercontent.com/85645967/131819643-6df9a4cc-4ffb-4b67-a42f-d7c44cb009b8.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4653 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARWH5XJMVVVUQY2J6E7UXDT747WPANCNFSM5CEFPQDQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

Santhosh-Sellavel · 2021-09-02T09:34:07Z

@shawnborton
Can you confirm on the Expensify card too?

Thanks!

shawnborton · 2021-09-02T09:52:07Z

That looks good for now, but I can see us wanting to add a more branded image instead of the gray icon. We can discuss in Slack?

…

On Thu, Sep 2, 2021 at 11:34 AM Santhoshkumar Sellavel < ***@***.***> wrote: @shawnborton <https://github.com/shawnborton> Can you confirm on the Expensify card too? [image: Screenshot 2021-09-02 at 3 03 13 PM] <https://user-images.githubusercontent.com/85645967/131820350-cfd4926c-d8c8-49de-8319-742160b2229a.png> Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4653 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARWH5VRQ4AI6LQQMZQ2SKDT75AJVANCNFSM5CEFPQDQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

Santhosh-Sellavel · 2021-09-02T09:57:41Z

expensifyCard is in assetes/images/expensifycard.svg

@shawnborton This was the image mentioned by @stitesExpensify

We can discussion in Slack?

Sure please initiate @shawnborton

shawnborton · 2021-09-02T10:16:26Z

Yes yes, I am saying the image is absolutely correct but now that I see the bank logos, perhaps we will want to update our own logo. Let’s solve that as a separate PR & discussion though, so feel free to keep marching forward in the meantime.

…

On Thu, Sep 2, 2021 at 11:57 AM Santhoshkumar Sellavel < ***@***.***> wrote: expensifyCard is in assetes/images/expensifycard.svg @shawnborton <https://github.com/shawnborton> This was the image mentioned by @stitesExpensify <https://github.com/stitesExpensify> We can discussion in Slack? Sure please initiate @shawnborton <https://github.com/shawnborton> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4653 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARWH5VFM7W4E6SXZUMDHTLT75DCBANCNFSM5CEFPQDQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

stitesExpensify

Looks great! Thanks for all of your work on this. One small change then you can take it off WIP and fill out the screenshots/tests.

Once that's done I'll approve and merge!

src/components/Icon/BankIcons.js

Santhosh-Sellavel · 2021-09-03T20:06:54Z

@stitesExpensify Ready for the review!

stitesExpensify

Looks great!

OSBotify · 2021-09-03T22:06:22Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2021-09-04T01:01:16Z

🚀 Deployed to staging by @stitesExpensify in version: 1.0.93-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

botify · 2021-09-08T20:56:43Z

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-09-15. 🎊

OSBotify · 2021-09-08T21:17:59Z

🚀 Deployed to production by @yuwenmemon in version: 1.0.94-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Santhosh-Sellavel added 2 commits August 14, 2021 01:04

Bank Icons add for payment methods initial commit

1e078ec

card bankName property renamed bank

558290f

Fixed Lint errors

cc18252

stitesExpensify requested changes Aug 17, 2021

View reviewed changes

assets/images/bankicons/discover.svg Outdated Show resolved Hide resolved

src/components/Icon/BankIcons.js Show resolved Hide resolved

Santhosh-Sellavel added 2 commits August 21, 2021 23:33

Merge branch 'main' into Add_Bank_Icons_To_Payment_Method

5439b6d

Discover Icon updated & Bank Icons size updated

fe374e3

Santhosh-Sellavel changed the title ~~[WIP] Bank Icons add for payment methods initial commit~~ [WIP] Bank Icons add for payment methods Aug 23, 2021

Santhosh-Sellavel requested a review from stitesExpensify August 26, 2021 03:37

Santhosh-Sellavel mentioned this pull request Aug 31, 2021

Add bank icons to the PaymentMethodsList #4398

Closed

stitesExpensify reviewed Aug 31, 2021

View reviewed changes

Resolved Merge conflict

650ae83

Added Expensify card, Default card & other review changes.

00f7e9c

Santhosh-Sellavel force-pushed the Add_Bank_Icons_To_Payment_Method branch from dda2f18 to 00f7e9c Compare September 2, 2021 07:01

stitesExpensify requested changes Sep 2, 2021

View reviewed changes

src/components/Icon/BankIcons.js Outdated Show resolved Hide resolved

Added iconSize in getBankIcon method

1063c23

Santhosh-Sellavel requested a review from stitesExpensify September 3, 2021 17:33

Santhosh-Sellavel marked this pull request as ready for review September 3, 2021 17:33

Santhosh-Sellavel requested a review from a team as a code owner September 3, 2021 17:33

MelvinBot requested review from nickmurray47 and removed request for a team September 3, 2021 17:33

stitesExpensify mentioned this pull request Sep 3, 2021

Feature: Transfer balance #4177

Closed

5 tasks

stitesExpensify changed the title ~~[WIP] Bank Icons add for payment methods~~ Bank Icons add for payment methods Sep 3, 2021

stitesExpensify approved these changes Sep 3, 2021

View reviewed changes

stitesExpensify merged commit 7f66480 into Expensify:main Sep 3, 2021

OSBotify mentioned this pull request Sep 3, 2021

Deploy Checklist: New Expensify 2021-09-03 #5086

Closed

14 tasks

kavimuru mentioned this pull request Sep 6, 2021

Settings - Bank Name is not saved when adding bank account #5104

Closed

stitesExpensify mentioned this pull request Sep 9, 2021

Settings - Bank Name is not saved when adding bank account #5168

Closed

Bank Icons add for payment methods #4653

Bank Icons add for payment methods #4653

Conversation

Santhosh-Sellavel commented Aug 13, 2021 • edited Loading

Details

Fixed Issues

Tests & QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Santhosh-Sellavel commented Aug 13, 2021

stitesExpensify left a comment

Choose a reason for hiding this comment

Santhosh-Sellavel commented Aug 17, 2021

Santhosh-Sellavel commented Aug 17, 2021

stitesExpensify commented Aug 17, 2021

Santhosh-Sellavel commented Aug 21, 2021

Android

web

Santhosh-Sellavel commented Aug 26, 2021

Santhosh-Sellavel commented Aug 27, 2021

stitesExpensify commented Aug 27, 2021

Santhosh-Sellavel commented Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stitesExpensify commented Aug 31, 2021

shawnborton commented Sep 1, 2021

stitesExpensify commented Sep 1, 2021

Santhosh-Sellavel commented Sep 2, 2021

shawnborton commented Sep 2, 2021 via email

Santhosh-Sellavel commented Sep 2, 2021 • edited Loading

shawnborton commented Sep 2, 2021 via email

Santhosh-Sellavel commented Sep 2, 2021 • edited Loading

shawnborton commented Sep 2, 2021 via email

Santhosh-Sellavel commented Sep 2, 2021

shawnborton commented Sep 2, 2021 via email

Santhosh-Sellavel commented Sep 2, 2021

shawnborton commented Sep 2, 2021 via email

stitesExpensify left a comment

Choose a reason for hiding this comment

Santhosh-Sellavel commented Sep 3, 2021

stitesExpensify left a comment

Choose a reason for hiding this comment

OSBotify commented Sep 3, 2021

OSBotify commented Sep 4, 2021

botify commented Sep 8, 2021

OSBotify commented Sep 8, 2021

Santhosh-Sellavel commented Aug 13, 2021 •

edited

Loading

Santhosh-Sellavel commented Aug 27, 2021 •

edited

Loading

Santhosh-Sellavel commented Sep 2, 2021 •

edited

Loading

Santhosh-Sellavel commented Sep 2, 2021 •

edited

Loading