From 3301b5850a5dffacb0b308efad5e010f73038fd5 Mon Sep 17 00:00:00 2001
From: Maxim Colls <collsmaxim@gmail.com>
Date: Sat, 17 Nov 2018 18:48:10 +0100
Subject: [PATCH 1/3] Improved UX in the accordion steps in the checkout page

---
 .../checkout/accordion_controller.js.coffee   | 25 ++++++++-----------
 .../checkout/billing_controller.js.coffee     |  4 +--
 .../checkout/details_controller.js.coffee     |  6 ++---
 .../darkswarm/mixins/fieldset_mixin.js.coffee |  4 +--
 app/views/checkout/edit.html.haml             |  2 +-
 5 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/app/assets/javascripts/darkswarm/controllers/checkout/accordion_controller.js.coffee b/app/assets/javascripts/darkswarm/controllers/checkout/accordion_controller.js.coffee
index e85f9e5cee..d61ff6ceb7 100644
--- a/app/assets/javascripts/darkswarm/controllers/checkout/accordion_controller.js.coffee
+++ b/app/assets/javascripts/darkswarm/controllers/checkout/accordion_controller.js.coffee
@@ -1,25 +1,22 @@
 Darkswarm.controller "AccordionCtrl", ($scope, localStorageService, $timeout, $document, CurrentHub) ->
-  key = "accordion_#{$scope.order.id}#{CurrentHub.hub.id}#{$scope.order.user_id}"
-  value = if localStorageService.get(key) then {} else { details: true, billing: false, shipping: false, payment: false }
-  localStorageService.bind $scope, "accordion", value, key
   $scope.accordionSections = ["details", "billing", "shipping", "payment"]
-  # Scrolling is confused by our position:fixed top bar - add an offset to scroll
-  # to the correct location, plus 5px buffer
-  offset_height = $("nav.top-bar").height() + 5
+  $scope.accordion = { details: true, billing: true, shipping: true, payment: true }
 
-  $scope.show = (section)->
+  $scope.show = (section) ->
     $scope.accordion[section] = true
-    # If we call scrollTo() directly after show(), when one of the accordions above the
-    # scroll location is closed by show(), scrollTo() will scroll to the old location of
-    # the element. Putting this in a 50 ms timeout is enough delay for the DOM to
-    # have updated.
-    $timeout ->
-      $document.scrollTo($("##{section}"), offset_height, 500)
-    , 50
+
+  $scope.scrollTo = (section) ->
+    # Scrolling is confused by our position:fixed top bar - add an offset to scroll
+    # to the correct location, plus 5px buffer
+    offset_height = $("nav.top-bar").height() + 5
+    $document.scrollTo($("##{section}"), offset_height, 400)
 
   $scope.$on 'purchaseFormInvalid', (event, form) ->
     # Scroll to first invalid section
     for section in $scope.accordionSections
       if not form[section].$valid
         $scope.show section
+        $timeout ->
+          $scope.scrollTo(section)
+        , 50
         break
diff --git a/app/assets/javascripts/darkswarm/controllers/checkout/billing_controller.js.coffee b/app/assets/javascripts/darkswarm/controllers/checkout/billing_controller.js.coffee
index b8dec8b190..b7246fddd9 100644
--- a/app/assets/javascripts/darkswarm/controllers/checkout/billing_controller.js.coffee
+++ b/app/assets/javascripts/darkswarm/controllers/checkout/billing_controller.js.coffee
@@ -5,7 +5,7 @@ Darkswarm.controller "BillingCtrl", ($scope, $timeout) ->
 
   $scope.summary = ->
     [$scope.order.bill_address.address1,
-    $scope.order.bill_address.city, 
+    $scope.order.bill_address.city,
     $scope.order.bill_address.zipcode]
 
-  $timeout $scope.onTimeout 
+  $timeout $scope.onTimeout
diff --git a/app/assets/javascripts/darkswarm/controllers/checkout/details_controller.js.coffee b/app/assets/javascripts/darkswarm/controllers/checkout/details_controller.js.coffee
index e8a6b1ff23..30c243ed7c 100644
--- a/app/assets/javascripts/darkswarm/controllers/checkout/details_controller.js.coffee
+++ b/app/assets/javascripts/darkswarm/controllers/checkout/details_controller.js.coffee
@@ -13,11 +13,11 @@ Darkswarm.controller "DetailsCtrl", ($scope, $timeout, $http, CurrentUser, Authe
 
   $scope.summary = ->
     [$scope.fullName(),
-    $scope.order.email, 
+    $scope.order.email,
     $scope.order.bill_address.phone]
 
   $scope.fullName = ->
-    [$scope.order.bill_address.firstname ? null, 
+    [$scope.order.bill_address.firstname ? null,
     $scope.order.bill_address.lastname ? null].join(" ").trim()
 
-  $timeout $scope.onTimeout 
+  $timeout $scope.onTimeout
diff --git a/app/assets/javascripts/darkswarm/mixins/fieldset_mixin.js.coffee b/app/assets/javascripts/darkswarm/mixins/fieldset_mixin.js.coffee
index 739d2de9bd..d8bb1b262e 100644
--- a/app/assets/javascripts/darkswarm/mixins/fieldset_mixin.js.coffee
+++ b/app/assets/javascripts/darkswarm/mixins/fieldset_mixin.js.coffee
@@ -2,11 +2,11 @@ window.FieldsetMixin = ($scope)->
   $scope.next = (event = false)->
     event.preventDefault() if event
     return unless $scope.nextPanel
+    $scope.accordion[$scope.name] = false
     $scope.show $scope.nextPanel
 
   $scope.onTimeout = ->
-    if $scope[$scope.name].$valid
-      $scope.next()
+    $scope.accordion[$scope.name] = !$scope[$scope.name].$valid
 
   $scope.valid = ->
     $scope.form().$valid
diff --git a/app/views/checkout/edit.html.haml b/app/views/checkout/edit.html.haml
index 75c9eff08f..ab5253538b 100644
--- a/app/views/checkout/edit.html.haml
+++ b/app/views/checkout/edit.html.haml
@@ -16,7 +16,7 @@
 
   = render partial: "shopping_shared/details"
 
-  %accordion{"close-others" => "true"}
+  %accordion{"close-others" => "false"}
     %checkout.row{"ng-controller" => "CheckoutCtrl"}
       .small-12.medium-8.large-9.columns
         - unless spree_current_user

From 884d4d0122d1627287055310d1115ddfb8b26b0b Mon Sep 17 00:00:00 2001
From: Maxim Colls <collsmaxim@gmail.com>
Date: Sat, 17 Nov 2018 19:46:09 +0100
Subject: [PATCH 2/3] Fixed specs

---
 .../consumer/shopping/checkout_spec.rb        | 34 ++++---------------
 .../shopping/embedded_shopfronts_spec.rb      |  3 --
 .../shopping/variant_overrides_spec.rb        |  4 ---
 3 files changed, 6 insertions(+), 35 deletions(-)

diff --git a/spec/features/consumer/shopping/checkout_spec.rb b/spec/features/consumer/shopping/checkout_spec.rb
index e4775cbdd3..ec4b86b698 100644
--- a/spec/features/consumer/shopping/checkout_spec.rb
+++ b/spec/features/consumer/shopping/checkout_spec.rb
@@ -64,19 +64,18 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
       let(:user) { create(:user) }
 
       def fill_out_form
-        toggle_shipping
         choose sm1.name
-        toggle_payment
         choose pm1.name
-        toggle_details
+
         within "#details" do
           fill_in "First Name", with: "Will"
           fill_in "Last Name", with: "Marshall"
           fill_in "Email", with: "test@test.com"
           fill_in "Phone", with: "0468363090"
         end
-        toggle_billing
+
         check "Save as default billing address"
+
         within "#billing" do
           fill_in "City", with: "Melbourne"
           fill_in "Postcode", with: "3066"
@@ -85,7 +84,6 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
           select "Victoria", from: "State"
         end
 
-        toggle_shipping
         check "Shipping address same as billing address?"
         check "Save as default shipping address"
       end
@@ -149,7 +147,6 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
         it "checks out successfully" do
           visit checkout_path
           choose sm2.name
-          toggle_payment
           choose pm1.name
 
           expect do
@@ -192,7 +189,6 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
 
           visit checkout_path
           fill_out_form
-          toggle_payment
           choose stripe_pm.name
         end
 
@@ -228,7 +224,6 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
       end
 
       it "shows a breakdown of the order price" do
-        toggle_shipping
         choose sm2.name
 
         page.should have_selector 'orderdetails .cart-total', text: with_currency(11.23)
@@ -242,7 +237,6 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
       end
 
       it "shows all shipping methods in order by name" do
-        toggle_shipping
         within '#shipping' do
           expect(page).to have_selector "label", count: 4 # Three shipping methods + instructions label
           labels = page.all('label').map(&:text)
@@ -254,7 +248,6 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
 
       context "when shipping method requires an address" do
         before do
-          toggle_shipping
           choose sm1.name
         end
         it "shows ship address forms when 'same as billing address' is unchecked" do
@@ -269,7 +262,6 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
 
         it "shows shipping methods allowed by the rule" do
           # No rules in effect
-          toggle_shipping
           page.should have_content "Frogs"
           page.should have_content "Donkeys"
           page.should have_content "Local"
@@ -315,7 +307,6 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
       before do
         visit checkout_path
         checkout_as_guest
-        toggle_payment
       end
 
       it "shows all available payment methods" do
@@ -326,8 +317,6 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
 
       describe "purchasing" do
         it "takes us to the order confirmation page when we submit a complete form" do
-          toggle_details
-
           within "#details" do
             fill_in "First Name", with: "Will"
             fill_in "Last Name", with: "Marshall"
@@ -335,8 +324,6 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
             fill_in "Phone", with: "0468363090"
           end
 
-          toggle_billing
-
           within "#billing" do
             fill_in "Address", with: "123 Your Face"
             select "Australia", from: "Country"
@@ -345,15 +332,11 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
             fill_in "Postcode", with: "3066"
           end
 
-          toggle_shipping
-
           within "#shipping" do
             choose sm2.name
             fill_in 'Any comments or special instructions?', with: "SpEcIaL NoTeS"
           end
 
-          toggle_payment
-
           within "#payment" do
             choose pm1.name
           end
@@ -382,18 +365,16 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
 
         context "with basic details filled" do
           before do
-            toggle_shipping
             choose sm1.name
-            toggle_payment
             choose pm1.name
-            toggle_details
+
             within "#details" do
               fill_in "First Name", with: "Will"
               fill_in "Last Name", with: "Marshall"
               fill_in "Email", with: "test@test.com"
               fill_in "Phone", with: "0468363090"
             end
-            toggle_billing
+
             within "#billing" do
               fill_in "City", with: "Melbourne"
               fill_in "Postcode", with: "3066"
@@ -401,7 +382,7 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
               select "Australia", from: "Country"
               select "Victoria", from: "State"
             end
-            toggle_shipping
+
             check "Shipping address same as billing address?"
           end
 
@@ -441,7 +422,6 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
               expect(page).to have_selector ".transaction-fee td", text: with_currency(0.00)
               expect(page).to have_selector ".total", text: with_currency(11.23)
 
-              toggle_payment
               choose "#{pm2.name} (#{with_currency(5.67)})"
 
               expect(page).to have_selector ".transaction-fee td", text: with_currency(5.67)
@@ -463,7 +443,6 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
                 let!(:pm1) { create(:payment_method, distributors: [distributor], name: "Roger rabbit", type: gateway_type) }
 
                 it "takes us to the order confirmation page when submitted with a valid credit card" do
-                  toggle_payment
                   fill_in 'Card Number', with: "4111111111111111"
                   select 'February', from: 'secrets.card_month'
                   select (Date.current.year+1).to_s, from: 'secrets.card_year'
@@ -478,7 +457,6 @@ feature "As a consumer I want to check out my cart", js: true, retry: 3 do
                 end
 
                 it "shows the payment processing failed message when submitted with an invalid credit card" do
-                  toggle_payment
                   fill_in 'Card Number', with: "9999999988887777"
                   select 'February', from: 'secrets.card_month'
                   select (Date.current.year+1).to_s, from: 'secrets.card_year'
diff --git a/spec/features/consumer/shopping/embedded_shopfronts_spec.rb b/spec/features/consumer/shopping/embedded_shopfronts_spec.rb
index 39a79aba2e..80f7e52eee 100644
--- a/spec/features/consumer/shopping/embedded_shopfronts_spec.rb
+++ b/spec/features/consumer/shopping/embedded_shopfronts_spec.rb
@@ -67,7 +67,6 @@ feature "Using embedded shopfront functionality", js: true do
           fill_in "Phone", with: "0456789012"
         end
 
-        toggle_billing
         within "#billing" do
           fill_in "Address", with: "123 Street"
           select "Australia", from: "Country"
@@ -76,12 +75,10 @@ feature "Using embedded shopfront functionality", js: true do
           fill_in "Postcode", with: "3066"
         end
 
-        toggle_shipping
         within "#shipping" do
           find('input[type="radio"]').trigger 'click'
         end
 
-        toggle_payment
         within "#payment" do
           find('input[type="radio"]').trigger 'click'
         end
diff --git a/spec/features/consumer/shopping/variant_overrides_spec.rb b/spec/features/consumer/shopping/variant_overrides_spec.rb
index 49d1b0b2e4..ff101fd5a0 100644
--- a/spec/features/consumer/shopping/variant_overrides_spec.rb
+++ b/spec/features/consumer/shopping/variant_overrides_spec.rb
@@ -173,7 +173,6 @@ feature "shopping with variant overrides defined", js: true, retry: 3 do
       fill_in "Phone", with: "0456789012"
     end
 
-    toggle_billing
     within "#billing" do
       fill_in "Address", with: "123 Street"
       select "Australia", from: "Country"
@@ -182,12 +181,10 @@ feature "shopping with variant overrides defined", js: true, retry: 3 do
       fill_in "Postcode", with: "3066"
     end
 
-    toggle_shipping
     within "#shipping" do
       choose sm.name
     end
 
-    toggle_payment
     within "#payment" do
       choose pm.name
     end
@@ -201,5 +198,4 @@ feature "shopping with variant overrides defined", js: true, retry: 3 do
       wait_until_enabled 'li.cart a.button'
       first(:link, 'Checkout now').click
   end
-
 end

From 4b588cbfb0d141af9ad913ac38fc6f3845d1f200 Mon Sep 17 00:00:00 2001
From: Maxim Colls <collsmaxim@gmail.com>
Date: Fri, 23 Nov 2018 16:47:55 +0000
Subject: [PATCH 3/3] Removed unused toggle helpers in spec/support

---
 spec/support/request/checkout_workflow.rb | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/spec/support/request/checkout_workflow.rb b/spec/support/request/checkout_workflow.rb
index a38edaa6de..11a13194bc 100644
--- a/spec/support/request/checkout_workflow.rb
+++ b/spec/support/request/checkout_workflow.rb
@@ -18,16 +18,4 @@ module CheckoutWorkflow
   def toggle_details
     toggle_accordion :details
   end
-
-  def toggle_billing
-    toggle_accordion :billing
-  end
-
-  def toggle_shipping
-    toggle_accordion :shipping
-  end
-
-  def toggle_payment
-    toggle_accordion :payment
-  end
 end