Skip to content

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Aug 28, 2018

The following tasks have been completed:

  • Confirmed there are no ReSpec errors/warnings.
  • Modified Web platform tests (link)
  • Modified MDN Docs (link)

Implementation commitment:

Optional, Impact on Payment Handler spec?


Preview | Diff

@marcoscaceres
Copy link
Member Author

@romandev, I saw you were helping implementing .retry() on the Chrome side. Is that correct?

Copy link
Member

@romandev romandev left a 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>,
Copy link
Member

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>,
Copy link
Member

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
Copy link
Member

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">

@marcoscaceres
Copy link
Member Author

@romandev wrote:

Yep, I'm implementing retry() feature in Chromium under @rsolomakhin's high-quality review.

Excellent to hear! What do you think of the proposed fix here? Make sense from an implementation perspective?

Copy link
Member

@romandev romandev left a 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.

@marcoscaceres
Copy link
Member Author

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.

I think it's fine, tbh.

In this case, should we allow shippingAddressErrors to be updated on payer detail change event?

I think we should allow it - just to reduce our (implementation) complexity. It's not different from a developer randomly invalidating their form elements.

@marcoscaceres
Copy link
Member Author

This is implemented in Firefox as part of https://bugzilla.mozilla.org/show_bug.cgi?id=1435161

@marcoscaceres
Copy link
Member Author

@romandev, can I get implementation commitment from the Blink side?

@romandev
Copy link
Member

@marcoscaceres Sure. I'm working on it.

@marcoscaceres
Copy link
Member Author

@romandev awesome to hear. When you can, could you drop add a tracking bug? 🙏

@romandev
Copy link
Member

I also filed this in Chromium :)
https://bugs.chromium.org/p/chromium/issues/detail?id=884433

@marcoscaceres
Copy link
Member Author

We've implemented this in Firefox. Landing this with @romandev's r+.

@marcoscaceres marcoscaceres merged commit 4020346 into gh-pages Sep 19, 2018
@marcoscaceres marcoscaceres deleted the fix_PaymentDetailsUpdate branch September 19, 2018 09:03
@aestes
Copy link
Collaborator

aestes commented Nov 2, 2018

We've implemented this in WebKit: https://trac.webkit.org/r236552

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PaymentDetailsUpdate needs to include payerErrors
3 participants