Skip to content

Removed conflicts from Jake Archibald pull request#327

Merged
ianbjacobs merged 3 commits intogh-pagesfrom
jake_ian_merge_2018108
Oct 17, 2018
Merged

Removed conflicts from Jake Archibald pull request#327
ianbjacobs merged 3 commits intogh-pagesfrom
jake_ian_merge_2018108

Conversation

@ianbjacobs
Copy link
Copy Markdown
Contributor

@ianbjacobs ianbjacobs commented Oct 8, 2018

#305

The following tasks have been completed:

  • web platform tests (link)
  • MDN Docs added (link)

Implementation commitment:

  • Safari (link to issue)
  • Chrome (link to issue)
  • Firefox (link to issue)
  • Edge (public signal)

Preview | Diff

@ianbjacobs ianbjacobs requested a review from rsolomakhin October 8, 2018 15:55
@ianbjacobs
Copy link
Copy Markdown
Contributor Author

Hi @rsolomakhin and @jakearchibald,

I endeavored to resolve the merge conflicts between Jake's pull request:
#305

and the current draft of the spec.

@rsolomakhin, I removed the bits you suggested be removed. I did not (yet) review the full algorithm to see whether it still makes sense in light of the edits.

Ian

Copy link
Copy Markdown
Collaborator

@rsolomakhin rsolomakhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but need to preserve the fields in PaymentRequestEvent.

Comment thread index.html
<dd>
The result of executing the <a>MethodData Population
Algorithm</a>
</dd>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep total, paymentRequestId, instrumentKey, and modifiers in the PaymentRequestEvent for now.

Suggested change
</dd>
</dd>
<dt>
<a data-lt="PaymentRequestEvent.total">total</a>
</dt>
<dd>
A <a>structured clone</a> of the total field on
the <a>PaymentDetailsInit</a> from the corresponding
<a>PaymentRequest</a>.
</dd>
<dt>
<a data-lt="PaymentRequestEvent.paymentRequestId">paymentRequestId</a>
</dt>
<dd>
The [[\details]].<a>id</a> from the <a>PaymentRequest</a>.
</dd>
<dt>
<a data-lt="PaymentRequestEvent.instrumentKey">instrumentKey</a>
</dt>
<dd>
The <a>instrumentKey</a> of the selected <a>PaymentInstrument</a>,
or the empty string if none was selected.
</dd>
<dt>
<a data-lt="PaymentRequestEvent.modifiers">modifiers</a>
</dt>
<dd>
The result of executing the <a>Modifiers Population Algorithm</a>.
</dd>

Comment thread index.html
<dd>
The result of executing the <a>Modifiers Population
Algorithm</a>.
</dd>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move these fields from canmakepayment event to paymentrequest event, please?

  • total
  • paymentRequestId
  • instrumentKey

Copy link
Copy Markdown
Collaborator

@rsolomakhin rsolomakhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost missed this :-)

The field names in canmakepayment event should be associated with CanMakePaymentEvent, not the PaymentRequestEvent.

Comment thread index.html Outdated
</p>
<dl>
<dt>
<a data-lt="PaymentRequestEvent.topOrigin">topOrigin</a>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<a data-lt="PaymentRequestEvent.topOrigin">topOrigin</a>
<a data-lt="CanMakePaymentEvent.topOrigin">topOrigin</a>

Comment thread index.html Outdated
</dd>
<dt>
<a data-lt=
"PaymentRequestEvent.paymentRequestOrigin">paymentRequestOrigin</a>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"PaymentRequestEvent.paymentRequestOrigin">paymentRequestOrigin</a>
"CanMakePaymentEvent.paymentRequestOrigin">paymentRequestOrigin</a>

Comment thread index.html Outdated
PaymentRequest was initialized.
</dd>
<dt>
<a data-lt="PaymentRequestEvent.methodData">methodData</a>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<a data-lt="PaymentRequestEvent.methodData">methodData</a>
<a data-lt="CanMakePaymentEvent.methodData">methodData</a>

Comment thread index.html Outdated
selected.
</dd>
<dt>
<a data-lt="PaymentRequestEvent.modifiers">modifiers</a>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<a data-lt="PaymentRequestEvent.modifiers">modifiers</a>
<a data-lt="CanMakePaymentEvent.modifiers">modifiers</a>

Copy link
Copy Markdown
Collaborator

@rsolomakhin rsolomakhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ianbjacobs ianbjacobs merged commit be353e8 into gh-pages Oct 17, 2018
@ianbjacobs ianbjacobs deleted the jake_ian_merge_2018108 branch October 17, 2018 19:02
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.

2 participants