-
Notifications
You must be signed in to change notification settings - Fork 138
Add paymentMethodErrors, payerErrors, to PaymentDetailsUpdate #768
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
Conversation
@romandev, I saw you were helping implementing |
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.
Yep, I'm implementing retry()
feature in Chromium under @rsolomakhin's high-quality review.
index.html
Outdated
Similarly, if <var>details</var>["<a>payerErrors</a>"] | ||
member is present and | ||
<var>request</var>.<a>[[\options]]</a>'s <a data-lt= | ||
"PaymentOptions.requestShipping">requestPayerName</a>, |
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.
nit: <a data-lt="PaymentOptions.requestPayerName">
index.html
Outdated
<var>request</var>.<a>[[\options]]</a>'s <a data-lt= | ||
"PaymentOptions.requestShipping">requestPayerName</a>, | ||
<a data-lt= | ||
"PaymentOptions.requestShipping">requestPayerEmail</a>, |
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.
nit: <a data-lt="PaymentOptions.requestPayerEmail">
index.html
Outdated
<a data-lt= | ||
"PaymentOptions.requestShipping">requestPayerEmail</a>, | ||
or <a data-lt= | ||
"PaymentOptions.requestShipping">requestPayerPhone</a> is |
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.
nit: <a data-lt="PaymentOptions.requestPayerPhone">
@romandev wrote:
Excellent to hear! What do you think of the proposed fix here? Make sense from an implementation perspective? |
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.
Sorry for delayed reply.
This seems reasonable to me for consistency.
One question is whether the some error values can be updated via updateWith()
on an unrelated event.
For example, requestShipping
and requestPayerName
are all true and merchant calls updateWith()
on payer detail change event as follows.
response.addEventListener('payerdetailchange', e => {
e.updateWith({
shippingAddressErrors: { city: 'CITY ERROR' }
});
});
In this case, should we allow shippingAddressErrors
to be updated on payer detail change event?
FYI, in Chromium implementation, if the shippingAddressErrors
is present, shipping address editor will perform after updateWith()
calls.
Thank you.
I think it's fine, tbh.
I think we should allow it - just to reduce our (implementation) complexity. It's not different from a developer randomly invalidating their |
This is implemented in Firefox as part of https://bugzilla.mozilla.org/show_bug.cgi?id=1435161 |
@romandev, can I get implementation commitment from the Blink side? |
@marcoscaceres Sure. I'm working on it. |
@romandev awesome to hear. When you can, could you drop add a tracking bug? 🙏 |
I also filed this in Chromium :) |
We've implemented this in Firefox. Landing this with @romandev's r+. |
We've implemented this in WebKit: https://trac.webkit.org/r236552 |
The following tasks have been completed:
Implementation commitment:
Optional, Impact on Payment Handler spec?
Preview | Diff