-
Notifications
You must be signed in to change notification settings - Fork 135
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
Merged PaymentRequest params and tweaked to address some issues #133
Conversation
# Conflicts: # specs/paymentrequest.html
|
||
<p>The <code>amount</code> sequence contains the amount (possibly in different currencies) that the website | ||
is requesting to be paid.</p> | ||
|
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.
Addresses #3
Also resolves #48 |
sequence<ShippingOption> shippingOptions; | ||
dictionary PaymentMethod { | ||
required sequence<DOMString> identifiers; | ||
object data; | ||
}; |
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.
Could add a sequence<CurrencyAmount> amount
to solve issue #4
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'm very wary of catch-all containers such as "data". I'd rather we figure out how we do extensibility properly before punting to "generic data buckets". So, -1 to this line. The rest of the PR is looking good - still reviewing it.
The separation of concerns that is introduced by this change also makes solving #1 more elegant. |
@@ -224,7 +225,7 @@ | |||
<section> | |||
<h2>PaymentRequest interface</h2> | |||
<pre class="idl"> | |||
[Constructor(sequence<DOMString> supportedMethods, PaymentDetails details, optional PaymentOptions options, optional object data)] | |||
[Constructor(PaymentDetails details, optional PaymentOptions options)] |
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, this is a very good step in the right direction.
👍 To this PR, but note that I'm in agreement with Manu's comments. |
<pre class="example highlight"> | ||
["visa", "bitcoin", "bobpay.com"] | ||
{ | ||
"amount" : [{ |
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 am not a fan of providing multi currency support, especially from a UX perspective. If a merchant really wants this, they could always ask their users in what currency they want to pay before calling paymentrequest.
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.
@zkoch We have had strong support for including this.
I understand that it makes the UX more complex but that's an implementer challenge that I assumed would be embraced as a way to differentiate.
Off the top of my head I can imagine a browser having a small selector that allows the user to select their payment currency and when they do the rest of the UI adjusts to display the pricing in that currency. The selector would only be visible if the request actually contains more than one currency.
On the other hand, I don't see us having any conformance language that forces the implementer to display all currencies to the user in the app selection UI. If the browser picks one currency and displays that currency and price that is still valid as the user's option to select another currency will then be made in the app.
I'm not in favour of this change. The motivation for the four arguments in the current API was as follows:
As I've mentioned before, the final In general in the web platform, we have APIs that consume domain specific data rather than one large undefined blob of data. I think it is better to keep the arguments separate and consider who will change them and when. |
@adrianba I understand the motivations and agree they are valid but I don't believe they justify the inflexibility that results from this format. Consider the following simple example. A new input parameter is defined for basic card payments. In general merchants will use the same value for all card payments. Today they would need to list out all of the card payment method identifiers they support in the So you have this: var request = new PaymentRequest(
["visa", "mastercard", "amex"],
details,
options,
{
"visa" : {
"customDataThing" : true,
},
"mastercard" : {
"customDataThing" : true,
},
"amex" : {
"customDataThing" : true,
},
}
); Instead of this: var request = new PaymentRequest(
options,
[
"identifiers" : ["visa", "mastercard", "amex"],
"data" : {
"customDataThing" : true,
}
]
); If you multiply that out for a merchant that supports more than 3 card types and you have a horrible mess. While I see the value in isolating data like What this proposal also addresses is the idea of "messaging" and the need to define a single JSON-serialized payment request message that is 1) not clouded with data that is only useful to the browser (like shipping options or line items) and 2) can be passed over process or network boundaries to a payment app. I assert that it is a requirement of the API that the website has complete control over the content of the payment request that is ultimately received by the payment app. i.e.This should not be modified in any way by the user agent. This suggests that this payment request message should be passed as a single object to the API and that any other data that is only required by the user agent should be put into one or more other parameters. |
@adrianhopebailie said:
+1 to this. This data really needs to be compartmentalized in something that can be a "message". Personally I would like to see the message signed in some way so that the payment app knows it was not altered on the way, but I know that is unlikely so I am just going to sit on that one until I get to say "told ya' so!" |
Merged PaymentRequest constructor params to allow for options (only
processed by the UA) and details (passed to the payment app).
This commit proposes solutions to:
#3, #4, #15 and #18