Closed Bug 1410321 Opened 2 years ago Closed 2 years ago

Decide on a framework for dynamic UI changes to the payment request dialog

Categories

(Firefox :: WebPayments UI, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: MattN, Assigned: MattN)

Details

User Story

* Site calls updateWith (only after shippingaddresschange/shippingoptionchange):
** supportedMethods change
** Shipping options change (e.g. based on the shipping address)
** Items change:
*** Items added/removed (e.g. taxes/shipping)
*** Highlight changed items

* Address/CC storage changes (e.g. from the management UI we open in a new tab)

State to preserve during updates:
* what screen the user is on when the update happens
* selected shipping method (if still offered and site didn't change .selected?)
* selected shipping address
* selected payment method (if still supported)
* selected payment method's billing address
* which items already exist to compare with new items to highlight new/changed
* item scroll positions
* field focus position

Need to disable some UI while an update is happening (see [[updating]] slot)

Attachments

(2 files)

Since there are various ways the payment request dialog can change once shown, we should keep that in mind for the architecture and setup a framework to makes this easier to maintain.

updateWith: https://w3c.github.io/payment-request/#dom-paymentrequestupdateevent-updatewith
I think virtual DOM diffing will be useful to avoid:
a) having to re-render the whole dialog when there is a change, or
b) having to hard-code logic to decide what UI needs to be updated for each change.

Components would be nice to encapsulate business logic close to the UI it's related to in order to keep the code maintainable.

I will attach a WIP patch using React to see if it makes sense to consider.
Comment on attachment 8920481 [details]
Bug 1410321 - WIP: React with Redux for Payment Request

This is still very minimal and doesn't really demonstrate use of Redux yet.
User Story: (updated)
After doing a lot of research into React/hyperHTML/lit-html/Polymer/CustomElements/Redux/etc., reading lots of blog posts, and talking to bgrins involved in the XBL replacement, I think we should go ahead with the Custom Elements approach of attachment 8920481 [details]. The main selling points:
* doesn't require any build or build tools to do development, all one needs is the html/css/js files in the res/ directory. It can be launched with a file:// URI, no server required.
* faster iterations by simply refreshing the page
* no indirection from third-party libraries makes debugging easier i.e. no libraries on the call stack
* aligns with the XBL replacement direction
* don't need to worry about keeping up with an evolving third-party library. Web Standards by their nature are quite static and any breakage from our CE implementation would be noticed quickly on automation.

Implementation using this approach will be done in separate bugs.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Comment on attachment 8924777 [details]
Bug 1410321 - Prototype: Custom Elements with Redux-like global state for the payment request dialog

https://reviewboard.mozilla.org/r/196014/#review204402

Initial review... haven't quite finished, so will finish it off tomorrow. Jet lag is slowing me down :(

::: toolkit/components/payments/content/paymentDialog.js:135
(Diff revision 4)
>      window.close();
>    },
>  
> +  pay(details) {
> +    let {payerName, payerEmail, payerPhone, paymentMethodName, paymentMethodData} = details;
> +    let basicCardData = this.createBasicCardResponseData(paymentMethodData);

nit: maybe rename this `basicCardResponse`?

::: toolkit/components/payments/content/paymentDialog.xhtml:14
(Diff revision 4)
>    <link rel="stylesheet" href="chrome://payments/content/paymentDialog.css"/>
>  </head>
>  
>  <body>
>    <iframe type="content"
>            id="paymentRequestFrame"

q: how come this needs a name if it's the id is the same? Is named used somewhere?

::: toolkit/components/payments/paymentUIService.js:29
(Diff revision 4)
>                                     "paymentSrv",
>                                     "@mozilla.org/dom/payments/payment-request-service;1",
>                                     "nsIPaymentRequestService");
>  
> +XPCOMUtils.defineLazyGetter(this, "profileStorage", () => {
> +  let _profileStorage;

nit: using _ is a bit odd, as that's usually convention for "privite member". Is this convention used elsewhere?

::: toolkit/components/payments/paymentUIService.js:133
(Diff revision 4)
> +   */
> +  closeDialog(requestId) {
> +    let enu = Services.wm.getEnumerator(null);
> +    let win;
> +    while ((win = enu.getNext())) {
> +      if (win.name == `${this.REQUEST_ID_PREFIX}${requestId}`) {

nit: ===

::: toolkit/components/payments/res/components.js:10
(Diff revision 4)
> +/**
> + * State-less presentational components.
> + *
> + * Do not connect to state.
> + */
> +

We should probably put each class into its own file.

::: toolkit/components/payments/res/components.js:23
(Diff revision 4)
> +    for (let name of (this.constructor.observedAttributes || [])) {
> +      if (name in this) {
> +        // Don't overwrite existing properties.
> +        continue;
> +      }
> +      Object.defineProperty(this, name.replace(/-([a-z])/g, ($0, $1) => $1.toUpperCase()), {

Please leave a comment here explaining what the regex replace is doing the to property name and why.

::: toolkit/components/payments/res/components.js:39
(Diff revision 4)
> +        },
> +      });
> +    }
> +  }
> +
> +  attributeChangedCallback(attr, oldValue, newValue) {

What's the rationale for not just using `attributeChangedCallback` to directly operate on the attributes instead of having the property getters/setters handling the mutations above? Seems cleaner to just use `attributeChangedCallback`... I'm still new to components, so just wondering?

::: toolkit/components/payments/res/components.js:65
(Diff revision 4)
> +    this._currencyEl.currency = this.getAttribute("currency");
> +    this._currencyEl.value = this.getAttribute("value");
> +  }
> +}
> +
> +customElements.define("payment-item", PaymentItem);

To avoid potential collisions in the future, maybe we should `mzpay-*` prefix things?
Comment on attachment 8924777 [details]
Bug 1410321 - Prototype: Custom Elements with Redux-like global state for the payment request dialog

https://reviewboard.mozilla.org/r/196014/#review204402

> q: how come this needs a name if it's the id is the same? Is named used somewhere?

Hmm… it was used before I think but I guess this is left over.

> nit: using _ is a bit odd, as that's usually convention for "privite member". Is this convention used elsewhere?

Yeah, this is unusual… I copied this from attachment 8896485 [details] by Jonathan and didn't notice then.

> nit: ===

Our team prefers == unless === is needed

> We should probably put each class into its own file.

Yeah, I agree and will do that. It was easier to move quickly in one file for prototyping.

> Please leave a comment here explaining what the regex replace is doing the to property name and why.

OK, I've done that in bug 1417749 now.

> What's the rationale for not just using `attributeChangedCallback` to directly operate on the attributes instead of having the property getters/setters handling the mutations above? Seems cleaner to just use `attributeChangedCallback`... I'm still new to components, so just wondering?

The setters have the advantage of sanely handling null/undefined whereas setAttribute does not. setAttribute("foo", null) causes the string "null" to be set as the attribute instead of removing the attribute.

> To avoid potential collisions in the future, maybe we should `mzpay-*` prefix things?

I originally had a prefix on all custom elements but it got quite verbose. Since these are scoped inside their own frame I think we can leave out the prefix for now.
Product: Toolkit → Firefox
You need to log in before you can comment on or make changes to this bug.