-
Notifications
You must be signed in to change notification settings - Fork 498
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
Integrate PaymentRequest (aka Web Payments) into Shop #70
Conversation
Features included are: * 'Buy Now' button added to detail pages when PaymentRequest (PR) is available. * All checkout buttons (e.g., from added-to-cart modal and on /cart) now check for PR and fire if available. * /checkout checks for PR on load and fires if available * Shipping options demonstrate having different options for different locations, including declining to ship outside of US.
What about PaymentRequest Polymer element (web component)? ;) Something like:
|
@frankiefu @keanulee @blasten WDYT? |
FYI, I'm in the process of factoring this out into a shop-payment-request component as @FluorescentHallucinogen suggests. Updated PR will be coming soon. |
I believe that Web Payment API is an important part of Progressive Web Apps. See "The Mobile Web Is Open for Business" article for more info: https://goo.gl/eIBP3a What about releasing full-featured Web Payment API web component? It can be part of Polymer Gold Elements ( |
Based on what's in gold and platinum already, Gold seems like a solid fit. Since I'm most of the way along factoring this out into |
This is awesome! |
I love hearing this!...Great job dude!! |
I tried https://web-payments-sample.firebaseapp.com/ and the code, but if I click on buy now or checkout, it seems to keep redirecting indefinitely. |
@blasten Looks like there was one remaining case where you could end up in an infinite loop trying to redirect to /checkout when PaymentRequest isn't available. I've addressed it here and pushed the change. |
woah, love it! |
@afitz0, any progress on factoring this out into a |
@FluorescentHallucinogen, yup, plenty of progress. :) Just been waylaid by a number of other things recently. Should have an updated PR early next week. |
PR updated to have most of the code in a |
@afitz0, well done! The code of Could you please fix this? |
A bit off topic. Anybody tried Notice the camera icon? Unfortunately, I have no camera icon. WAIDW? |
@tjsavage, @blasten, @frankiefu, I repeat my question. What about official full-featured Web Payment API web component? It can be part of Polymer Gold Elements ( WDYT? Any plans? |
@FluorescentHallucinogen latest commit now permits shipping to non-US addresses. https://web-payments-sample.firebaseapp.com/ has been updated with the latest code (though you may need to clear caches). |
LGTM |
If the user click "Buy now" button, all items previously added to cart will be removed. Is it expected behavior? |
1. When user taps "buy now" from item detail 2. When user tries to check out (cart was being erroneously re-initialized)
@FluorescentHallucinogen expected, but not desired. I've fixed that as well as a related but separate issue with the cart being incorrectly re-initialized. |
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.
@afitz0 Thanks a lot for the PR! I left a few comments.
} else if (err.name === 'NotSupportedError') { | ||
that._redirectToCheckout(); | ||
} else { | ||
console.error('Error while handling PaymentRequest.show(): ' + err); |
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.
In Chrome M53 and Canary, I receive an error of type UnknownError
from the request API. Would it make sense to just check for err === 'Request cancelled'
and call _redirectToCheckout
otherwise?
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.
That error, AFAICT, happens in the case that window.PaymentRequest
exists but isn't actually implemented (as is the case in Chrome M53+ on !Android if you have chrome://flags#enable-experimental-web-platform-features
on). A small edge case, for sure, but easy enough to handle. Done.
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.
great!
@@ -19,6 +19,7 @@ | |||
<link rel="import" href="../bower_components/iron-pages/iron-pages.html"> | |||
<link rel="import" href="../bower_components/iron-selector/iron-selector.html"> | |||
<link rel="import" href="shop-category-data.html"> | |||
<link rel="import" href="shop-payment-request.html"> |
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.
This seems like a good candidate for a lazy import. This import could be moved to src/lazy-resources.html
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.
Done.
@@ -366,6 +379,7 @@ | |||
'add-cart-item': '_onAddCartItem', | |||
'set-cart-item': '_onSetCartItem', | |||
'clear-cart': '_onClearCart', | |||
'process-payment': '_onProcessPayment', |
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: You could add this event listener to shop-payment-request
. e.g. <shop-payment-request on-process-payment="_onProcessPayment" ..>
That way, uses would know that the event came from shop-payment-request
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.
Done!
shop-payment-request provides the logic and hooks into the PaymentRequest API. | ||
--> | ||
<shop-payment-request | ||
page="[[page]]" |
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.
Would it be possible to make this element a bit more general? One idea could be:
- shop-payment-request exposes a
request
method. That method takes an array of items (cart items), the total and the currency. - shop-payment-request fires non-bubbling events for
success
&error
. For theerror
event, the event's detail could contain the error type. e.g. 0: not supported, 1: request canceled, 2: other.
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.
Generalized it a bit, following your suggestion. A quick followup could be (though I haven't yet) to pull out _updateShippingAddress
into an app-specific callback, similar to process-payment
as it has a lot of app-specific logic (i.e., logic around shipping costs).
size: this.$.sizeSelect.value | ||
}; | ||
|
||
this.fire('buy-item', item); |
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.
The buy-item
event could be handled by show-app
which then can call this.$.payment.request();
assuming <shop-payment-request id="payment" ...>
Also buy-item
might not need the item in the event's detail.
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.
Unless I'm not seeing something in the event, buy-item
needs the item included so that it can process the payment for that item in isolation. Otherwise we get into the business of cart manipulation for handling a single item, and I've opted to leave the cart alone for the "Buy Now" feature.
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 see the intention now. That works as well.
cart="[[cart]]" | ||
total="[[total]]" | ||
currency="USD" | ||
supported-methods='["visa", "mastercard", "amex", "discover", "maestro", "diners", "jcb", "unionpay", "bitcoin"]' |
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:this could be the default for supported-methods
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.
Done.
@@ -23,6 +23,19 @@ | |||
|
|||
<style include="shop-common-styles shop-button shop-select"> | |||
|
|||
shop-button#buyNow > * { |
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.
q: is shop-button#buyNow > *
needed to increase the specificity of the selector? would #buyNow > button
work?
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.
Yup. Fixed.
@@ -493,6 +496,12 @@ <h2 id="billAddressHeading">Billing Address</h2> | |||
} | |||
}, | |||
|
|||
_cartChanged: function() { | |||
if (this.cart && this.cart.length > 0) { |
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: this check could be done in shop-app
if you decide to handle buy-cart
in that element.
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.
Done.
width: 100%; | ||
} | ||
|
||
#buyNow:not([visible]) { |
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: it could be
[hidden] {
display: none;
}
and hidden$="[[!_hasPaymentRequest()]]"
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.
Fixed.
_redirectToCheckout: function() { | ||
// prevent reload loop if we're already on /checkout. | ||
if (this.page !== 'checkout') { | ||
location.href = '/checkout'; |
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.
The redirection seems to be reloading the app. If the redirection were handled in shop-app
, it would be possible to get a reference to the app-location
and change the route: e.g. this.$.location.set('route.path', '/');
wdyt?
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.
Makes sense to me. Moved to fire from shop-app
.
@afitz0, would you mind updating your PR? |
https://web-payments-sample.firebaseapp.com has been demonstrated alongside https://polykart-credential-payment.appspot.com at Polymer Summit 2016 — https://youtu.be/NDZw7vtoYnU :) |
* Move shop-payment-request to lazy import * Shortend supported payment list (full list set as default) * Declare process-payment event as coming from shop-payment-request
* `shop-payment-request` is a bit more general now (e.g., by decoupling it from the cart's structure and the logic around "Buy Now" vs. "Buy Cart") * `_redirectToCheckout` in `shop-payment-request` no longer causes an app reload. * `shop-payment-request` fires coded `error` events on failure, and redirects for _all_ errors except `Request Cancelled`
@agektmr, could you please publish the source code of https://polykart-credential-payment.appspot.com? |
@FluorescentHallucinogen Yes, the code is continuation of @afitz0 as you may know. I need a little clean up but will send a PR once it's sorted. |
@FluorescentHallucinogen Which branch should I send a PR to? I need to rebase as I'm not sure from which point @afitz0 has forked this. |
@agektmr I forked off master, and the latest commits in this PR bring it in sync. |
on-process-payment="_onProcessPayment" | ||
on-error="_onPaymentError" | ||
supported-methods='["visa", "mastercard"]' | ||
android-pay='{"merchantName":"Android Pay Demo","merchantId":"12345678901234567890","environment":"TEST","allowedCardNetworks":["AMEX","DISCOVER","MASTERCARD","VISA"],"paymentMethodTokenizationParameters":{"tokenizationType":"GATEWAY_TOKEN","parameters":{"gateway":"stripe","stripe:publishableKey":"pk_live_abcdefghijklmnopqrstuvwx","stripe:version":"2016-07-06"}}}'> |
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.
this is a comment for myself since that might require a bit of refactoring: this could be move to a config file.
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.
@afitz0 Sorry for the delay I took a week off. I left a few comments and I think this PR should be ready to merge. Hopefully, they shouldn't take that long.
if ('PaymentRequest' in window && this.cart && this.numItems > 0) { | ||
var itemDetails = []; | ||
|
||
for (var i = 0; i < this.cart.length; ++i) { |
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: Array.map
instead of for
loop.
|
||
request: function(itemDetails, totalCost, currency, clearCartOnSuccess) { | ||
if (!'PaymentRequest' in window) { | ||
this._redirectToCheckout(); |
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 we fire the error
event? That way, we could simplify this operation to a success/error message, then _onError
could redirect depending on the status code.
A proposal:
- Remove
_redirectToCheckout
- Fire
error
and specify theerror_code
.
Also, this method could return a promise in the future.
if (err === 'Request cancelled') { | ||
that.fire('error', {error_code: 0, message: err}); | ||
} else if (err.name == 'NotSupportedError') { | ||
that.fire('error', {error_code: 1, message: err.message}); |
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: 1
could be a constant. e.g. Polymer.ShopPaymentRequest.NOT_SUPPORTED = 1;
that way the user could check for them in the error handler.
A proposal:
Polymer.ShopPaymentRequest = Polymer({ is: 'shop-payment-request', ... });
Polymer.ShopPaymentRequest.NOT_SUPPORTED = 1;
...
...
} | ||
}); | ||
} catch (e) { | ||
this._redirectToCheckout(); |
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.
Fire error
event.
return items; | ||
}, | ||
|
||
_redirectToCheckout: function() { |
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.
it could be removed
@@ -283,6 +325,10 @@ | |||
if (!offline) { | |||
this._tryReconnect(); | |||
} | |||
}, | |||
|
|||
_hasPaymentRequest: function() { |
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.
it could be removed since the contract would just be this.$.payment.request()
and the error/success events.
--> | ||
<shop-payment-request | ||
id="payment" | ||
on-process-payment="_onProcessPayment" |
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.
Minor nit: wdyt if we rename this event as success
? My thinking is that it would be easier to grasp since the user would know that this is a binary operation.
I think this PR is deprecated and probably should be closed. Instead #91 is the one extending from this and have more stuffs. |
Could #91 be rebased after making the changes here? @afitz0, @agektmr, @frankiefu wdyt? I can move the comments to #91 if that helps. |
If @afitz0 is ok, forking, making changes and sending PR to https://github.com/agektmr/shop/tree/credential-payment might be easier for me. |
Fantastic, looking forward to it! |
Closing this as #91 is the continuation of this. |
Features included are:
This code is currently hosted here: https://web-payments-sample.firebaseapp.com/