-
Notifications
You must be signed in to change notification settings - Fork 44
Card Form in PayPalWebPayments #321
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
base: bcdc-feature
Are you sure you want to change the base?
Conversation
I like including the branded card checkout within the
When we build the native card fields solution, we can put it in it's own module because it will encompass BCDC, ACDC, and the other payment method options (PayPal, Pay Later, Credit, and more 👀 ). |
@nirvan543 yes i think it makes sense to have it here. I considered having it as a separate library thinking that might be a new feature from the merchant/customer's point of view and something we could keep the same function signature for when we implement native. I'm thinking about how we'd present this in our docs. If you want to be able to use card forms, we have guest checkout Is it confusing for people who want to use this module for PayPal checkout flows only to see "card" value in But I agree, this flow makes more sense from code perspective. As long as it doesn't cause confusion for the merchants, this definitely makes more sense. |
|
||
func testInit_whenCardButtonCreated_hasUIImageFromAssets() { | ||
let sut = CardButton() | ||
XCTAssertEqual(sut.imageView?.image, UIImage(named: "card-black")) |
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.
This test, I found out is not testing what it's supposed to test on all the other buttons as well.
It just passes because both values are nil. I will make a ticket to fix these tests.
XCTAssertEqual(sut.edges, PaymentButtonEdges.softEdges) | ||
XCTAssertEqual(sut.size, PaymentButtonSize.collapsed) | ||
XCTAssertEqual(sut.color, PaymentButtonColor.black) | ||
XCTAssertEqual(sut.label, PaymentButtonLabel.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.
Because this button logo doesn't have text, i defaulted it to having the label.
So collapsed and mini don't show "Debit or Credit Card" label text
Full and expanded show the label text.
All the other buttons default to collapsed, those buttons have PayPal text embedded in the logo, I'm wondering default size on the card button should be the collapsed or something else.
print("configResult: \(configResult)") | ||
} catch { | ||
print("error in calling graphQL: \(error.localizedDescription)") | ||
self.notifyCheckoutFailure(with: PayPalError.payPalURLError, completion: completion) |
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.
Probably should not return with failure. Not sure about situation where this call would fail.
@@ -33,6 +33,17 @@ extension PayPalCreditButton.Color { | |||
} | |||
} | |||
|
|||
extension CardButton.Color { |
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.
q: Are we able to use CaseIterable
to autogenerate this code? link: docs.swift.org
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.
The extensions in the demo app were done that way because those enums are declared in the SDK and we can't add CaseIterable conformance from outside the module. This iteration is not something the user would do, but kind of specific to our demo app to showcase all possible button appearances
@@ -7,4 +7,5 @@ public enum HTTPHeader: String { | |||
case authorization = "Authorization" | |||
case contentType = "Content-Type" | |||
case origin = "Origin" | |||
case paypalClientContext = "paypal-client-context" |
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.
case paypalClientContext = "paypal-client-context" | |
case payPalClientContext = "paypal-client-context" |
nit: I'm honestly back and forth on which version is best.
"\(baseURLString)/checkoutnow?token=\(request.orderID)" + | ||
"&fundingSource=\(request.fundingSource.rawValue)" |
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.
"\(baseURLString)/checkoutnow?token=\(request.orderID)" + | |
"&fundingSource=\(request.fundingSource.rawValue)" | |
"\(baseURLString)/checkoutnow?token=\(request.orderID)" + | |
"&fundingSource=\(request.fundingSource.rawValue)" |
quickfix: indentation.
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.
🔥
…nd Steven PR feedback
Summary of changes
CardButton
PayPalWebCheckoutFundingSource
enumthis enables
start
function to distinguish the flows for API call (we need to get specs for)TODO:
Checklist
Authors