Bug 1437879 - Set the currency-amount properties as attributes on the shipping-option so they get copied to the cloned option. r?mattn draft
authorJared Wein <jwein@mozilla.com>
Tue, 13 Feb 2018 10:24:25 -0500
changeset 755000 26546b406d143e6d6cff95079819368a1f78585d
parent 754399 38b3c1d03a594664c6b32c35533734283c258f43
push id99093
push userbmo:jaws@mozilla.com
push dateWed, 14 Feb 2018 20:46:58 +0000
reviewersmattn
bugs1437879
milestone60.0a1
Bug 1437879 - Set the currency-amount properties as attributes on the shipping-option so they get copied to the cloned option. r?mattn MozReview-Commit-ID: 9rngciIXPkX
toolkit/components/payments/res/components/shipping-option.js
toolkit/components/payments/res/containers/shipping-option-picker.js
toolkit/components/payments/test/mochitest/test_shipping_option_picker.html
--- a/toolkit/components/payments/res/components/shipping-option.js
+++ b/toolkit/components/payments/res/components/shipping-option.js
@@ -9,40 +9,41 @@
  */
 
 /* global ObservedPropertiesMixin, RichOption */
 
 class ShippingOption extends ObservedPropertiesMixin(RichOption) {
   static get observedAttributes() {
     return RichOption.observedAttributes.concat([
       "label",
+      "amount-currency",
+      "amount-value",
     ]);
   }
 
   constructor() {
     super();
 
     this.amount = null;
-    this._amount = document.createElement("currency-amount");
-    this._amount.classList.add("amount");
+    this._currencyAmount = document.createElement("currency-amount");
+    this._currencyAmount.classList.add("amount");
     this._label = document.createElement("span");
     this._label.classList.add("label");
   }
 
   connectedCallback() {
-    this.appendChild(this._amount);
+    this.appendChild(this._currencyAmount);
     this.appendChild(this._label);
     super.connectedCallback();
   }
 
   render() {
-    // An amount is required to render the shipping option.
-    if (!this.amount) {
-      return;
-    }
-
-    this._amount.currency = this.amount.currency;
-    this._amount.value = this.amount.value;
     this._label.textContent = this.label;
+    this._currencyAmount.currency = this.amountCurrency;
+    this._currencyAmount.value = this.amountValue;
+    // Need to call render after setting these properties
+    // if we want the amount to get displayed in the same
+    // render pass as the label.
+    this._currencyAmount.render();
   }
 }
 
 customElements.define("shipping-option", ShippingOption);
--- a/toolkit/components/payments/res/containers/shipping-option-picker.js
+++ b/toolkit/components/payments/res/containers/shipping-option-picker.js
@@ -28,19 +28,19 @@ class ShippingOptionPicker extends Payme
     let {shippingOptions} = state.request.paymentDetails;
     let desiredOptions = [];
     for (let option of shippingOptions) {
       let optionEl = this.dropdown.getOptionByValue(option.id);
       if (!optionEl) {
         optionEl = document.createElement("shipping-option");
         optionEl.value = option.id;
       }
-      for (let attr of ["amount", "label"]) {
-        optionEl[attr] = option[attr];
-      }
+      optionEl.label = option.label;
+      optionEl.amountCurrency = option.amount.currency;
+      optionEl.amountValue = option.amount.value;
       desiredOptions.push(optionEl);
     }
     let el = null;
     while ((el = this.dropdown.popupBox.querySelector(":scope > shipping-option"))) {
       el.remove();
     }
     for (let option of desiredOptions) {
       this.dropdown.popupBox.appendChild(option);
--- a/toolkit/components/payments/test/mochitest/test_shipping_option_picker.html
+++ b/toolkit/components/payments/test/mochitest/test_shipping_option_picker.html
@@ -75,19 +75,28 @@ add_task(async function test_initialSet(
       },
     },
     selectedShippingOption: "456",
   });
   await asyncElementRendered();
   let options = picker1.dropdown.popupBox.children;
   is(options.length, 2, "Check dropdown has both options");
   ok(options[0].textContent.includes("Carrier Pigeon"), "Check first option");
-  is(options[0].querySelector(".amount").getAttribute("currency"), "USD", "Check currency");
+  is(options[0].querySelector(".amount").currency, "USD", "Check currency");
   ok(options[1].textContent.includes("Lightspeed (default)"), "Check second option");
   is(picker1.dropdown.selectedOption, options[1], "Lightspeed selected by default");
+
+  let selectedClone = picker1.dropdown.querySelector(".rich-select-selected-clone");
+  let text = selectedClone.textContent;
+  ok(text.includes("$20.00"),
+     "Shipping option clone should include amount. Value = " + text);
+  ok(text.includes("Lightspeed (default)"),
+     "Shipping option clone should include label. Value = " + text);
+  ok(!isHidden(selectedClone),
+     "Shipping option clone should be visible");
 });
 
 add_task(async function test_update() {
   picker1.requestStore.setState({
     request: {
       paymentDetails: {
         shippingOptions: [
           {
@@ -113,17 +122,17 @@ add_task(async function test_update() {
     },
     selectedShippingOption: "456",
   });
 
   await promiseStateChange(picker1.requestStore);
   let options = picker1.dropdown.popupBox.children;
   is(options.length, 2, "Check dropdown still has both options");
   ok(options[0].textContent.includes("Tortoise"), "Check updated first option");
-  is(options[0].querySelector(".amount").getAttribute("currency"), "CAD", "Check currency");
+  is(options[0].querySelector(".amount").currency, "CAD", "Check currency");
   ok(options[1].textContent.includes("Lightspeed (default)"), "Check second option is the same");
   is(picker1.dropdown.selectedOption, options[1], "Lightspeed selected by default");
 });
 
 add_task(async function test_change_selected_option() {
   let options = picker1.dropdown.popupBox.children;
   let selectedOption = picker1.dropdown.selectedOption;
   is(options[1], selectedOption, "Should default to Lightspeed option");