Closed Bug 1427936 Opened 6 years ago Closed 6 years ago

Basic Payment Request Display Items list UI

Categories

(Firefox :: WebPayments UI, enhancement, P1)

Product:
Firefox
Component:
WebPayments UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: MattN, Assigned: sfoster)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webpayments])

Attachments

(1 file)

Bug 1427936 - Add an order-details component and details-payment-items.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
Render the `state.request.paymentDetails.displayItems` in a list format using the `PaymentStateSubscriberMixin` and add a toggle to the dialog to toggle the list view visibility.

Display Items should use the <currency-amount> custom element.
Blocks: 1427939
Assignee: nobody → sfoster
Blocks: 1429264
Comment on attachment 8941740 [details]
Bug 1427936 - Add an order-details component and details-payment-items.

https://reviewboard.mozilla.org/r/211978/#review217906

Here's some initial thoughts for you to consider in order to unblock you. I didn't do a full review of the CSS.

::: toolkit/components/payments/res/components/items-list.js:13
(Diff revision 1)
> +class PaymentItemsList extends ObservedPropertiesMixin(HTMLElement) {
> +
> +  static get observedAttributes() {
> +    return [];
> +  }

`ObservedPropertiesMixin` seems unused.

::: toolkit/components/payments/res/components/items-list.js:24
(Diff revision 1)
> +    let row = document.createElement("div");
> +    row.classList.add("row");
> +    let label = document.createElement("div");

Semantically we should be using <ul> + <li> or <table>, rather than a <div>, I think. Did you consider making a custom element for a line?

::: toolkit/components/payments/res/components/items-list.js:38
(Diff revision 1)
> +  static _createDivider(fragment, className) {
> +    let element = document.createElement("hr");
> +    if (className) {
> +      element.classList.add(className);
> +    }
> +    fragment.appendChild(element);
> +    return element;
> +  }

I guess this was moved to CSS?

::: toolkit/components/payments/res/components/items-list.js:47
(Diff revision 1)
> +    }
> +    fragment.appendChild(element);
> +    return element;
> +  }
> +
> +  render(state) {

There is no `state` argument when you're not using the `PaymentStateSubscriberMixin`

::: toolkit/components/payments/res/components/order-details.js:13
(Diff revision 1)
> +
> +/**
> + * <order-details></order-details>
> + */
> +
> +class OrderDetails extends PaymentStateSubscriberMixin(HTMLElement) {

This should probably be moved into the res/containers/ directory since it's less about rendering the data, and more about the layout. I'm using a slightly different defintion than https://medium.com/@dan_abramov/smart-and-dumb-components-7ca2f9a7c7d0

::: toolkit/components/payments/res/components/order-details.js:31
(Diff revision 1)
> +  setLoadingState(state) {
> +    this.requestStore.setState(state);
> +  }

You shouldn't need this. This is just for loading initial state at the top level from the parent process.

::: toolkit/components/payments/res/components/order-details.js:36
(Diff revision 1)
> +    console.log("OrderDetails render");
> +    let request = state.request;
> +    console.log('OrderDetails render, request: ', request);

Perhaps you were going to delete this console.log before landing but I think we should have logging in the unprivileged part that respects the existing loglevel pref used by the privileged code since it will ease development to be able to leave some logging in place.

One idea was to use a Proxy on the console object that checks the pref and returns an empty function for log levels greater than the loglevel but I also heard on dev-platform that Proxies are expensive… Alternatively we could have our `log` object which conditionally forwards to the appropriate `console` method depending on the pref.

::: toolkit/components/payments/res/components/order-details.js:55
(Diff revision 1)
> +  }
> +
> +  static isTaxItem(item) {
> +    // XXX: label string comparison is a placeholder,
> +    // pending resolution of https://github.com/w3c/payment-request/issues/163
> +    return item.type === 'tax' || item.label == "Tax";

Nit: use == and double quotes

::: toolkit/components/payments/res/paymentRequest.xhtml:50
(Diff revision 1)
> +    <payment-items-list id="display-items-list" empty-label="No items"></payment-items-list>
> +    <payment-items-list id="sticky-display-items-list" class="sticky"></payment-items-list>

I'm not sure I like having two separate components for this since. Two separate <tbody> in one <table> could work
Attachment #8941740 - Flags: review?(MattN+bmo)
> Semantically we should be using <ul> + <li> or <table>, rather than a <div>,
> I think. Did you consider making a custom element for a line?

I went back and forth on this. I guess in the absence of better arguments, its tabular data so it makes sense to be a table. If a compelling reason emerges for it not to be a table we can change it at that time. 
I did consider making a line element but didn't see it adding much value right now. I'm happy to break that out if it turns out to be needed. 

Sorry you got more fluff/cruft in this patch than I expected. I'll get it cleaned up in the next push. 

> > +class OrderDetails extends PaymentStateSubscriberMixin(HTMLElement) {
> 
> This should probably be moved into the res/containers/ directory since it's
> less about rendering the data, and more about the layout. I'm using a
> slightly different defintion than
> https://medium.com/@dan_abramov/smart-and-dumb-components-7ca2f9a7c7d0

Ok, yeah I wasn't sure how you were defining containers. 

> ::: toolkit/components/payments/res/paymentRequest.xhtml:50
> (Diff revision 1)
> > +    <payment-items-list id="display-items-list" empty-label="No items"></payment-items-list>
> > +    <payment-items-list id="sticky-display-items-list" class="sticky"></payment-items-list>
> 
> I'm not sure I like having two separate components for this since. Two
> separate <tbody> in one <table> could work

I'll try that. I was confused earlier about where the second list's items were coming from and the decision to treat it as 2 lists stemmed from that.
Comment on attachment 8941740 [details]
Bug 1427936 - Add an order-details component and details-payment-items.

https://reviewboard.mozilla.org/r/211978/#review217906

> Semantically we should be using <ul> + <li> or <table>, rather than a <div>, I think. Did you consider making a custom element for a line?

I tried this as a <table>, but it makes styling onerous. The current patch uses an UL/payment-details-item for semantics, and grid and display: contents to lay out the item contents.

> Perhaps you were going to delete this console.log before landing but I think we should have logging in the unprivileged part that respects the existing loglevel pref used by the privileged code since it will ease development to be able to leave some logging in place.
> 
> One idea was to use a Proxy on the console object that checks the pref and returns an empty function for log levels greater than the loglevel but I also heard on dev-platform that Proxies are expensive… Alternatively we could have our `log` object which conditionally forwards to the appropriate `console` method depending on the pref.

I've removed console.logs for now, but I guess we should have a bug for logging and a conversation about what logging would be useful.

> I'm not sure I like having two separate components for this since. Two separate <tbody> in one <table> could work

I'm currently going with a order-details component which is the store subscribe and container. And a payment-details-item component which represents the line-items in the table. I'm not sure this is right either, but its closer?
Matt: I updated the patch to add the payment-display-item tests. I've not done anything for order-details, as that is the most tentative. If the design/implementation seems reasonable (at least for now) I'll go ahead and start writing order-details tests.
See Also: → 1431482
Comment on attachment 8941740 [details]
Bug 1427936 - Add an order-details component and details-payment-items.

https://reviewboard.mozilla.org/r/211978/#review220654

I'm not able to test the patch/CSS because my build isn't working but I think this is good to continue.

::: toolkit/components/payments/res/components/payment-details-item.js:28
(Diff revision 4)
> +    for (let child of this.children) {
> +      child.remove();
> +    }

Since the elements should stay the same, I think we should create the elements in the constructor and simply modify them later.

::: toolkit/components/payments/res/components/payment-details-item.js:43
(Diff revision 4)
> +    if (!this.parentNode) {
> +      return;
> +    }

This seems like it may cause bugs if an attribute changes while the element isn't yet in the document but then it gets added to the document. I don't think you should need this.

::: toolkit/components/payments/res/components/payment-details-item.js:47
(Diff revision 4)
> +    amount.currency = this.amountCurrency || "";
> +    amount.value = this.amountValue || "";
> +
> +    let label = this.querySelector(":scope > .label");
> +    label.textContent = this.label || "";

For attributes that are required, defaulting to a value will hide bugs. Instead I think we should `throw` like currency-amount does: https://dxr.mozilla.org/mozilla-central/rev/c4ebc8c28a33b785dfbfa533810517cc707d1ad0/toolkit/components/payments/res/components/currency-amount.js#24

::: toolkit/components/payments/res/containers/order-details.js:26
(Diff revision 4)
> +  get mainItemsList() {
> +    return this.querySelector(":scope > .main-list");
> +  }
> +
> +  get stickyItemsList() {
> +    return this.querySelector(":scope > .sticky-list");
> +  }

Originally I was thinking we don't need two containers but I guess since only the main part scrolls then we need both.

::: toolkit/components/payments/res/containers/order-details.js:30
(Diff revision 4)
> +  get stickyItemsList() {
> +    return this.querySelector(":scope > .sticky-list");

Nit: I'm not really a fan of this name TBH… maybe footerItemsList?

::: toolkit/components/payments/res/containers/order-details.js:64
(Diff revision 4)
> +    this.totalAmountElem.value = totalItem.amount.value || 0;
> +    this.totalAmountElem.currency = totalItem.amount.currency || "";

Here too I think it's better to throw for required properties so we don't hide bugs.

::: toolkit/components/payments/test/mochitest/mochitest.ini:21
(Diff revision 4)
>     ../../res/vendor/custom-elements.min.js
>     ../../res/vendor/custom-elements.min.js.map
>     payments_common.js
>  
>  [test_currency_amount.html]
> +[test_payment_details_item.html]

You forgot to add this to your commit.
Attachment #8941740 - Flags: review?(MattN+bmo)
Comment on attachment 8941740 [details]
Bug 1427936 - Add an order-details component and details-payment-items.

https://reviewboard.mozilla.org/r/211978/#review220654

> Nit: I'm not really a fan of this name TBH… maybe footerItemsList?

Agreed, I didn't like it either but figured I or someone would come up with something better before landing.
Comment on attachment 8941740 [details]
Bug 1427936 - Add an order-details component and details-payment-items.

https://reviewboard.mozilla.org/r/211978/#review220654

> Since the elements should stay the same, I think we should create the elements in the constructor and simply modify them later.

I ran into a "Operation not supported" trying to do the element creation in the constuctor, so I'm doing that in the connectedCallback in the current patch.
(In reply to Sam Foster [:sfoster] from comment #12)
> Comment on attachment 8941740 [details]
> Bug 1427936 - Add an order-details component and details-payment-items.
> 
> https://reviewboard.mozilla.org/r/211978/#review220654
> 
> > Since the elements should stay the same, I think we should create the elements in the constructor and simply modify them later.
> 
> I ran into a "Operation not supported" trying to do the element creation in
> the constuctor, so I'm doing that in the connectedCallback in the current
> patch.

Hmm… was that when trying to append to the DOM? You should be able to create elements as long as you don't modify the children of the custom element being created (IIUC).
Comment on attachment 8941740 [details]
Bug 1427936 - Add an order-details component and details-payment-items.

https://reviewboard.mozilla.org/r/211978/#review222108

Thanks. Looks good overall, I mostly have suggestions about when we `createElement` still in order to avoid repeated querySelector on each `render`.

::: toolkit/components/payments/res/components/payment-details-item.js:11
(Diff revision 5)
> +                          amountValue="1.00"
> +                          amountCurrency="USD"></payment-details-item>

These should be with hyphens e.g. amount-value

::: toolkit/components/payments/res/components/payment-details-item.js:27
(Diff revision 5)
> +  connectedCallback() {
> +    let fragment = document.createDocumentFragment();
> +    let label = document.createElement("span");
> +    label.classList.add("label");
> +    fragment.appendChild(label);
> +
> +    let amount = document.createElement("currency-amount");
> +    fragment.appendChild(amount);
> +    this.appendChild(fragment);

This is going to create these new children every time the element is re-added to a document. You should create the elements (e.g. `this._currencyAmount` & `this._label`) with the appropriate attributes and store a reference in the constructor and then append those existing element references in the `connectedCallback`. Then in render you can refer to the members in `render` instead of using querySelector you can use the existing element references which should be more performant. I think Jared said there was a leak originally with this approach but I think that's a Gecko bug so we switched back to the shim for tests. See https://dxr.mozilla.org/mozilla-central/rev/e22e2c9eb81686e958a9448ea3d1e8cd766743e2/toolkit/components/payments/res/containers/address-picker.js#18,22 for an example.

::: toolkit/components/payments/res/components/payment-details-item.js:44
(Diff revision 5)
> +    const number = Number.parseFloat(this.amountValue);
> +    if (Number.isNaN(number) || !Number.isFinite(number)) {
> +      throw new RangeError("amount-value value must be a finite number");
> +    }
> +    amount.value = number;

What was the reason for checking this here rather than letting currency-amount handling it? Was it just so there was less indirection when the error occurs?

::: toolkit/components/payments/res/containers/order-details.css:28
(Diff revision 5)
> +order-details .footer-items-list:not(:empty):before {
> +  border: 1px solid GrayText;
> +  display: block;
> +  content: "";
> +  grid-column-start: 1;
> +  grid-column-end: 4;
> +}

Interesting… why not a single border instead of the pseudo element?

::: toolkit/components/payments/res/containers/order-details.css:48
(Diff revision 5)
> +.details-total > currency-amount {
> +  font-size: large;

Note: I'm pretty much ignoring visual styling in this patch as it's likely going to change. I'm generally trying to keep things as simple as possible for now.

::: toolkit/components/payments/res/containers/order-details.js:16
(Diff revision 5)
> +    this._template = document.getElementById("order-details-template");
> +  }
> +
> +  connectedCallback() {
> +    let contents = document.importNode(this._template.content, true);

I think you copied this from <payment-dialog> but could we instead do the import in the constructor and then we don't need to hold on to the template? Then we can assign our `this._mainItemsList` and stuff also in the constructor?

::: toolkit/components/payments/res/containers/order-details.js:26
(Diff revision 5)
> +  get mainItemsList() {
> +    return this.querySelector(":scope > .main-list");
> +  }
> +
> +  get footerItemsList() {
> +    return this.querySelector(":scope > .footer-items-list");
> +  }
> +
> +  get totalAmountElem() {
> +    return this.querySelector(":scope > .details-total > currency-amount");
> +  }

Since these are static return values these should also probably be static private member variables assigned to in the constructor.

::: toolkit/components/payments/res/containers/order-details.js:46
(Diff revision 5)
> +    items.forEach(item => {
> +      let row = document.createElement("payment-details-item");

Nit: our team generally prefers for…of loops over .forEach and I believe it has slightly less overhead due to no function call

::: toolkit/components/payments/res/containers/order-details.js:74
(Diff revision 5)
> +    const totalValue = Number.parseFloat(totalItem.amount.value);
> +    if (Number.isNaN(totalValue) || !Number.isFinite(totalValue)) {
> +      throw new RangeError("total amount value must be a finite number");
> +    }

Same question here as in the line element

::: toolkit/components/payments/res/containers/order-details.js:84
(Diff revision 5)
> +    this.totalAmountElem.value = totalValue;
> +    this.totalAmountElem.currency = totalItem.amount.currency;
> +  }
> +
> +  /**
> +   * Determine if an display item should belong in the footer list.

Typo: a/an/a/

::: toolkit/components/payments/test/mochitest/test_order_details.html:77
(Diff revision 5)
> +  let paymentDetails = requestStore.getState().request.paymentDetails;
> +  paymentDetails.displayItems = [
> +    {
> +      label: "One",
> +      amount: { currency: "USD", value: "5" },
> +    },
> +    {
> +      label: "Two",
> +      amount: { currency: "USD", value: "6" },
> +    },
> +    {
> +      label: "Three",
> +      amount: { currency: "USD", value: "7" },
> +    },
> +  ];

Btw. feel free to add good test cases of payment details to https://dxr.mozilla.org/mozilla-central/rev/e22e2c9eb81686e958a9448ea3d1e8cd766743e2/toolkit/components/payments/test/PaymentTestUtils.jsm#89 as those are shared between mochitest plain and b-c.
Attachment #8941740 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8941740 [details]
Bug 1427936 - Add an order-details component and details-payment-items.

https://reviewboard.mozilla.org/r/211978/#review222108

> This is going to create these new children every time the element is re-added to a document. You should create the elements (e.g. `this._currencyAmount` & `this._label`) with the appropriate attributes and store a reference in the constructor and then append those existing element references in the `connectedCallback`. Then in render you can refer to the members in `render` instead of using querySelector you can use the existing element references which should be more performant. I think Jared said there was a leak originally with this approach but I think that's a Gecko bug so we switched back to the shim for tests. See https://dxr.mozilla.org/mozilla-central/rev/e22e2c9eb81686e958a9448ea3d1e8cd766743e2/toolkit/components/payments/res/containers/address-picker.js#18,22 for an example.

OK that's exactly what I ended up doing after debugging this: creating the elements in the constructor and storing references to them, appending in connectedCallback and just using the references rather than querySelector in render.

> What was the reason for checking this here rather than letting currency-amount handling it? Was it just so there was less indirection when the error occurs?

I think this was to catch exceptions when rendering without initial/default values. But it doesn't add much value over the exception currency-amount will throw, so I'll remove it here.

> Interesting… why not a single border instead of the pseudo element?

No great reason, except the options and flexibility this gives us in how we end up styling it.

> I think you copied this from <payment-dialog> but could we instead do the import in the constructor and then we don't need to hold on to the template? Then we can assign our `this._mainItemsList` and stuff also in the constructor?

document.getElementById("order-details-template") returns null when I move it to the constructor; it seems this runs before DOMContentLoaded. I would need to rearrange the order of the templates and scripts for this to work?
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ea415aa3620
Add an order-details component and details-payment-items. r=MattN
https://hg.mozilla.org/mozilla-central/rev/6ea415aa3620
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1435316
Priority: P3 → P1
Whiteboard: [webpayments]
Product: Toolkit → Firefox
Target Milestone: mozilla60 → Firefox 60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: