-
Notifications
You must be signed in to change notification settings - Fork 612
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
card edit #1349
card edit #1349
Conversation
Autotagging @bigcommerce/storefront-team @davidchin |
config.json
Outdated
@@ -302,7 +302,1261 @@ | |||
"discover", | |||
"mastercard", | |||
"visa" | |||
] | |||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @junedkazi , as we decided to create the input fields manually for the card management feature, I created three new attributes on the config.json to add the default countries, states and form validations. I would like to know if this the right place for storing those values? Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we want to hard code the counties and states list ? Can't we make a call to bcapp and fetch the information ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I don't think we should be hardcoding this list. //cc @precariouspanther @zackangelo @chrisboulton another possible opportunity to use the new service that we have been discussing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please, we've had several occasions where we have had to update these lists - it'd be preferable to avoid fragmenting this. The other concern is these values need to be matched to names within our lists which change (rather than the ISO codes) so there is a risk that changes to the official list would be a backwards breaking change for this that we wouldn't realise.
Please consider driving this from either @aleachjr 's geo service or BCApp's current definition (Which will be migrated)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback guys, I'll discuss with the team and make the changes to provide "countries" and "states" from the bcapp. cc @bc-anna @jackosaurus
Hey @junedkazi @bc-zoharmuzafi , that PR is now ready for review. if you can have a look that would be awesome. Thanks! |
"card_ending_in": "ending in {last_four}", | ||
"card_expiry": "{month}/{year}", | ||
"new_payment_method": "Add new payment method", | ||
"no_methods": "You currently have no payment methods added to your account", | ||
"name_on_card": "Name on card", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we no longer will be showing name on the card ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @junedkazi , yes we realized the name on card
was not something we keep when vaulting the card. Hey @jackosaurus , please correct me if I'm wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct, we do not store "Name on card" (we were mistaken)
What?
Tickets
Screenshots (if appropriate)
Desktop
Mobile