Client handles payment method change #4
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
trcoffman
commented
Nov 22, 2021
trcoffman
commented
- Make the client of the library handle payment method changes
- Clean up the test suite
- Remove storing of credit and debit details from the PaymentRequest
We are no longer changing behavior for how PaymentRequest handles its details constructor argument, so the test changes from PR #1 are no longer valid. Revert these. There is a better way of changing PaymentRequestUpdateEvent, so revert all the changes from PR #1 and then re-do the tests in a following commit.
…vent names the same. In PaymentRequestUpdateEvent, all we have changed is added a new option for what the event name can be called. The simplest way to make sure that the class behaves the same for all valid event names is to simply run the entire test suite for all valid event names. So rather than duplicate select tests and give our 'paymentmethodchange' event, just use forEach to run the entire test suite for all valid event names.
trcoffman
commented
Nov 22, 2021
| @@ -158,7 +152,7 @@ export default class PaymentRequest { | |||
| // 6. If the displayItems member of details is present, then for each item in details.displayItems: | |||
| validateDisplayItems(details.displayItems, ConstructorError); | |||
|
|
|||
| // 7. Let selectedShippingOption be null. | |||
| // 7. Let selectedShippingOption and payment method be null. | |||
There was a problem hiding this comment.
This comment should not change because this isn't where i initialized _paymentMethod
dvicory
approved these changes
Nov 23, 2021
There was a problem hiding this comment.
This looks great, the diff between upstream is looking a lot better.
The following can be done here or in follow-ups:
- Clean up exposing
PKPaymentMethodType${type}in this PR or a quick follow-up. - The event structure for payment method change. The comment is wrong at the very least, but not sure if there's more guidance in the spec about what the structure/values should be.
| }).toThrow(); | ||
| }); | ||
| }); | ||
| ['shippingaddresschange', 'shippingoptionchange', 'paymentmethodchange'].forEach((eventName) => { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.