Closed Bug 1431482 Opened 2 years ago Closed 2 years ago

Create and hook up the "View all items" button

Categories

(Firefox :: WebPayments UI, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: sfoster, Assigned: sfoster)

References

Details

(Whiteboard: [webpayments])

Attachments

(1 file)

Clicking on "View all items" needs to show the container element which has the detailed display items list. Clicking again should hide the same.
Comment on attachment 8944028 [details]
Bug 1431482 - Add view-all button and toggle a activePanel property on the store.

https://reviewboard.mozilla.org/r/214344/#review220674

::: toolkit/components/payments/res/mixins/PaymentStateSubscriberMixin.js:17
(Diff revision 1)
>  
>  /**
>   * State of the payment request dialog.
>   */
>  let requestStore = new PaymentsStore({
> +  activePanel: "",

If we're thinking about this as switching between panels (which I'm not sure I love), then it feels a bit weird to use "" to represent the summary view. Personally I would prefer the model of the view all items as an overlay on top of the summary and use a boolean state. This feels like it would be a bit simpler to me.

::: toolkit/components/payments/res/paymentRequest.css:7
(Diff revision 1)
> +  height: 100%;
>  }
>  
> -#debugging-console {
> -  float: right;
> +body {
> +  width: 100%;
> +  height: 100%;
> +  padding: 0;
> +  margin: 0;

Some of this smells like things one does for cross-browser compatibility with old browsers and not things we need in Firefox.

::: toolkit/components/payments/res/paymentRequest.css:24
(Diff revision 1)
> +.hidden {
> +  display: none;
> +}

You should use the standard @hidden attribute instead. It tells screenreaders not to read the contents.

::: toolkit/components/payments/res/paymentRequest.css:74
(Diff revision 1)
> +#debugging-console {
> +  float: right;
> +}

I was trying to keep the CSS roughtly aligned with the DOM tree order of the body so that's why this was  first after `html`

::: toolkit/components/payments/res/paymentRequest.xhtml:45
(Diff revision 1)
> +      <section id="payment-summary-panel" class="panel">
> +        <h1>Your Payment</h1>

One thing I think we discussed on IRC is whether the details is a conceptually an overlay on top of the summary panel that is simply toggled or whether we think of it like tabs that you toggle between. This is implementing the latter. I think the latter is a bit simpler TBH but either can work.

::: toolkit/components/payments/res/paymentRequest.xhtml:51
(Diff revision 1)
> +    <footer id="controls-container">
>        <button id="cancel">Cancel</button>
>        <button id="pay">Pay</button>
> -    </div>
> +    </footer>

From the UX spec it seems like these won't show while viewing items
Attachment #8944028 - Flags: review?(MattN+bmo)
Comment on attachment 8944028 [details]
Bug 1431482 - Add view-all button and toggle a activePanel property on the store.

https://reviewboard.mozilla.org/r/214344/#review220674

> If we're thinking about this as switching between panels (which I'm not sure I love), then it feels a bit weird to use "" to represent the summary view. Personally I would prefer the model of the view all items as an overlay on top of the summary and use a boolean state. This feels like it would be a bit simpler to me.

OK, yeah I can go either way on this. Overlay it is.

> Some of this smells like things one does for cross-browser compatibility with old browsers and not things we need in Firefox.

Yeah, width: 100% is not necessary here; height on both html and body is needed however.
Comment on attachment 8944028 [details]
Bug 1431482 - Add view-all button and toggle a activePanel property on the store.

https://reviewboard.mozilla.org/r/214344/#review221162

::: toolkit/components/payments/res/containers/payment-dialog.js:82
(Diff revision 1)
>    render(state) {
>      let request = state.request;

According to the UX spec, we should hide the View All  button if there are no display items / additional display items: https://mozilla.invisionapp.com/share/YAFI31XR3KP#/screens/275361826

You can do this in this bug or a follow-up, or a separate commit in the same bug. Up to you.
Comment on attachment 8944028 [details]
Bug 1431482 - Add view-all button and toggle a activePanel property on the store.

https://reviewboard.mozilla.org/r/214344/#review220674

> OK, yeah I can go either way on this. Overlay it is.

Looking at the updated UX spec, it seems like the View All Items pane can also overlay the FTU add address flow so I think that pushes me even more towards the overlay approach. The View ALL Items overlay can appear over all but the intro page in the wizard flow (intro => add address FTU => add card FTU => summary). https://mozilla.invisionapp.com/share/YAFI31XR3KP#/screens/275361847
Comment on attachment 8944028 [details]
Bug 1431482 - Add view-all button and toggle a activePanel property on the store.

https://reviewboard.mozilla.org/r/214344/#review221162

> According to the UX spec, we should hide the View All  button if there are no display items / additional display items: https://mozilla.invisionapp.com/share/YAFI31XR3KP#/screens/275361826
> 
> You can do this in this bug or a follow-up, or a separate commit in the same bug. Up to you.

Oh good catch. It probably makes sense to address this when the additional display items work is done.
I made the changes to the patch to treat the order details content as an overlay. The CSS to lay all this out is sketched in, we'll presumably know more about how this should go once we start getting more content in here, and when we start thinking about any animated transitions (but if something is obviously wrong of course I can fix it.) 

As discussed in #payments, to test the view-all button and its state store interaction, I started a test_payment_dialog.html, with a copy of the payment-dialog's template (may need updating if this gets rebased before landing?)
Comment on attachment 8944028 [details]
Bug 1431482 - Add view-all button and toggle a activePanel property on the store.

https://reviewboard.mozilla.org/r/214344/#review221930

Thanks! Looks good overall.

::: toolkit/components/payments/res/containers/payment-dialog.js:94
(Diff revision 3)
> +    let detailsOverlay = this._orderDetailsOverlay;
> +    if (state.orderDetailsShowing) {
> +      detailsOverlay.removeAttribute("hidden");
> +    } else {
> +      detailsOverlay.setAttribute("hidden", "true");
> +    }

Nit: This can be simplified to one line:
```js
this._orderDetailsOverlay.hidden = !state.orderDetailsShowing;
```

::: toolkit/components/payments/res/paymentRequest.xhtml:57
(Diff revision 3)
> -    <div id="controls-container">
> +        <footer id="controls-container">
> -      <button id="cancel">Cancel</button>
> +          <button id="cancel">Cancel</button>
> -      <button id="pay">Pay</button>
> +          <button id="pay">Pay</button>
> +        </footer>
> +      </section>
> +      <section id="order-details-overlay" hidden="true">

Some people argue that one should repeat the attribute name as the value for XHTML boolean attributes so people don't get confused and think they can change "true" to "false" and have an effect. That would mean `hidden="hidden"` in this case (same as in the test). If this was regular HTML then we would leave the value out but since we'll probably use DTD for l10n we need to be XHTML.

::: toolkit/components/payments/test/mochitest/test_payment_dialog.html:20
(Diff revision 3)
> +  <template id="payment-dialog-template">
> +    <header>
> +      <div id="total">

Hmm… having to copy the <template>s is going to be more of a maintenance burden than just keeping the JS and CSS file references up-to-date… I don't really have a simple solution… 

Some ideas:
a) I guess we could have each template in its own file and the `fetch` it as a Document (equivalent of responseType = "document" with XHR) but then we'd have to wait for the templates to load.

b) Instead we could reference paymentRequest.xhtml from this test as a same-origin <iframe>. Not sure if CSP will get in the way of some things we'd want to do for the tests.

I don't think either of these need to block this from landing but we should figure out a solution before this gets too out-of-hand with many more templates.
Attachment #8944028 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8944028 [details]
Bug 1431482 - Add view-all button and toggle a activePanel property on the store.

https://reviewboard.mozilla.org/r/214344/#review221930

> Hmm… having to copy the <template>s is going to be more of a maintenance burden than just keeping the JS and CSS file references up-to-date… I don't really have a simple solution… 
> 
> Some ideas:
> a) I guess we could have each template in its own file and the `fetch` it as a Document (equivalent of responseType = "document" with XHR) but then we'd have to wait for the templates to load.
> 
> b) Instead we could reference paymentRequest.xhtml from this test as a same-origin <iframe>. Not sure if CSP will get in the way of some things we'd want to do for the tests.
> 
> I don't think either of these need to block this from landing but we should figure out a solution before this gets too out-of-hand with many more templates.

yeah I'll look into some options. If we were a web project, this would be a built-time problem (for better or worse.) And we would suffer the same trade-off - it would likely require some script to produce runnable assets and so would break our file:// URL + reload workflow.
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b610f06c36a
Add view-all button and toggle a activePanel property on the store. r=MattN
https://hg.mozilla.org/mozilla-central/rev/3b610f06c36a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Priority: P3 → P1
Whiteboard: [webpayments]
Product: Toolkit → Firefox
Target Milestone: mozilla60 → Firefox 60
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.