From 127eaa44e5a040ca258931a0d91176b022344a2a Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 29 Nov 2023 16:12:45 +1100
Subject: [PATCH 01/14] Ensure loading message always shows

Before, it was affixed near the top of the page and wasn't visible after scrolling down.
---
 app/views/admin/products_v3/index.html.haml     |  8 +++++---
 .../css/admin_v3/components/spinner.scss        | 17 ++++++++++++-----
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/app/views/admin/products_v3/index.html.haml b/app/views/admin/products_v3/index.html.haml
index 581dcc2605..82a4e5f38e 100644
--- a/app/views/admin/products_v3/index.html.haml
+++ b/app/views/admin/products_v3/index.html.haml
@@ -11,7 +11,9 @@
 = render partial: 'spree/admin/shared/product_sub_menu'
 
 #products_v3_page{ "data-controller": "products" }
-  #loading-spinner.spinner-container{ "data-controller": "loading", "data-products-target": "loading" }
-    .spinner
-    = t('.loading')
+
+  .spinner-overlay{ "data-controller": "loading", "data-products-target": "loading" }
+    .spinner-container
+      .spinner
+      = t('.loading')
   #products-content
diff --git a/app/webpacker/css/admin_v3/components/spinner.scss b/app/webpacker/css/admin_v3/components/spinner.scss
index d6694b3ed2..fd7235ffe3 100644
--- a/app/webpacker/css/admin_v3/components/spinner.scss
+++ b/app/webpacker/css/admin_v3/components/spinner.scss
@@ -1,16 +1,23 @@
-.spinner-container {
+.spinner-overlay {
   position: absolute;
-  width: 100%;
+  left: 0;
+  right: 0;
   height: 100%;
   min-height: 200px;
+  background: rgba(255, 255, 255, 0.8);
+  z-index: 2;
+}
+
+.spinner-container {
+  position: fixed;
+  left: 0;
+  right: 0;
   display: flex;
-  justify-content: flex-start;
+  justify-content: center;
   align-items: center;
   flex-direction: column;
   gap: 40px;
   font-size: 24px;
-  background: rgba(255, 255, 255, 0.8);
-  z-index: 2;
 
   &.hidden {
     display: none;

From 7d299affd3bd9515fba9c2cf8ba96e3b38a2bb57 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 30 Nov 2023 15:03:08 +1100
Subject: [PATCH 02/14] Move hiding logic to stimulus controller

This ensures morphed flashes hide like other flashes (eg in bulk order actions). I wanted to write a spec to prove it, but Capybara doesn't support mocking setTimeout and I didn't want to use sleep.

I've made it optional because this controller is shared with the shop frontend ([supposedly](https://github.com/openfoodfoundation/openfoodnetwork/commit/5ef34347a39a05c1dd2495109142b53246e135b4), although angular seems to override it).
---
 .../javascripts/admin/spree/base.js.erb       |  3 --
 app/views/admin/shared/_flashes.html.haml     |  2 +-
 app/webpacker/controllers/flash_controller.js | 17 ++++++++++
 app/webpacker/css/admin/animations.scss       | 16 +++++++--
 .../stimulus/flash_controller_test.js         | 34 +++++++++++++++++++
 5 files changed, 65 insertions(+), 7 deletions(-)
 create mode 100644 spec/javascripts/stimulus/flash_controller_test.js

diff --git a/app/assets/javascripts/admin/spree/base.js.erb b/app/assets/javascripts/admin/spree/base.js.erb
index 8a2015d786..99aada8cb5 100644
--- a/app/assets/javascripts/admin/spree/base.js.erb
+++ b/app/assets/javascripts/admin/spree/base.js.erb
@@ -32,9 +32,6 @@ jQuery(function($) {
     });
   }
 
-  // Make flash messages dissapear
-  setTimeout('$(".flash").fadeOut()', 5000);
-
   // Highlight hovered table column
   $('table tbody tr td.actions a').hover(function(){
     var tr = $(this).closest('tr');
diff --git a/app/views/admin/shared/_flashes.html.haml b/app/views/admin/shared/_flashes.html.haml
index 57673b3f48..1683d5a1d8 100644
--- a/app/views/admin/shared/_flashes.html.haml
+++ b/app/views/admin/shared/_flashes.html.haml
@@ -1,6 +1,6 @@
 #flashes
   - if defined? flashes
     - flashes.each do |type, msg|
-      .animate-show{"data-controller": "flash"}
+      .animate-show{"data-controller": "flash", "data-flash-auto-close-value": "true"}
         .flash{type: "#{type}", class: "#{type}"}
           %span= msg
diff --git a/app/webpacker/controllers/flash_controller.js b/app/webpacker/controllers/flash_controller.js
index c30c08bcab..100d34fa69 100644
--- a/app/webpacker/controllers/flash_controller.js
+++ b/app/webpacker/controllers/flash_controller.js
@@ -5,7 +5,24 @@ document.addEventListener("turbolinks:before-cache", () =>
 );
 
 export default class extends Controller {
+  static values = {
+    autoClose: Boolean,
+  };
+
+  connect() {
+    if (this.autoCloseValue) {
+      setTimeout(this.close.bind(this), 5000);
+    }
+  }
+
   close() {
+    // Fade out
+    this.element.classList.remove("animate-show");
+    this.element.classList.add("animate-hide-500");
+    setTimeout(this.remove.bind(this), 500);
+  }
+
+  remove() {
     this.element.remove();
   }
 }
diff --git a/app/webpacker/css/admin/animations.scss b/app/webpacker/css/admin/animations.scss
index 7b6e451078..6dffab1cfe 100644
--- a/app/webpacker/css/admin/animations.scss
+++ b/app/webpacker/css/admin/animations.scss
@@ -11,13 +11,23 @@
 }
 
 @keyframes fade-out-hide {
-  0%   {opacity: 1; visibility: visible;}
-  99%  {opacity: 0; visibility: visible;}
-  100% {opacity: 0; visibility: hidden;}
+  0% {
+    opacity: 1;
+    visibility: visible;
+  }
+  99% {
+    opacity: 0;
+    visibility: visible;
+  }
+  100% {
+    opacity: 0;
+    visibility: hidden;
+  }
 }
 
 .animate-hide-500 {
   animation: fade-out-hide 0.5s;
+  opacity: 0; // Stay invisible after animation
 }
 
 // @-webkit-keyframes slideOutDown
diff --git a/spec/javascripts/stimulus/flash_controller_test.js b/spec/javascripts/stimulus/flash_controller_test.js
new file mode 100644
index 0000000000..665c07aa47
--- /dev/null
+++ b/spec/javascripts/stimulus/flash_controller_test.js
@@ -0,0 +1,34 @@
+/**
+ * @jest-environment jsdom
+ */
+
+import { Application } from "stimulus";
+import flash_controller from "../../../app/webpacker/controllers/flash_controller";
+
+describe("FlashController", () => {
+  beforeAll(() => {
+    const application = Application.start();
+    application.register("flash", flash_controller);
+  });
+
+  beforeEach(() => {
+    document.body.innerHTML = `
+      <div id="element" data-controller='flash' data-flash-auto-close-value='true'></div>
+    `;
+
+  });
+
+  describe("autoClose", () => {
+    jest.useFakeTimers();
+
+    it("is cleared after about 5 seconds", () => {
+      let element = document.getElementById("element");
+      expect(element).not.toBe(null);
+
+      jest.advanceTimersByTime(5500);
+
+      element = document.getElementById("element");
+      expect(element).toBe(null);
+    });
+  });
+});

From 70f153b0f7c9bab2751b8308e01969bbe9eebfbc Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 1 Dec 2023 12:06:55 +1100
Subject: [PATCH 03/14] Refactor spec

Only the first example is testing the updated variant here.
---
 spec/system/admin/products_v3/products_spec.rb | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index b4294bdc4d..5e4537875c 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -298,14 +298,15 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
           fill_in "Name", with: ""
           fill_in "SKU", with: "A" * 256
         end
+      end
+
+      it "shows errors for both product and variant fields" do
+        # Update variant with invalid data too
         within row_containing_name("Medium box") do
           fill_in "Name", with: "L" * 256
           fill_in "SKU", with: "1" * 256
           fill_in "Price", with: "10.25"
         end
-      end
-
-      it "shows errors for both product and variant fields" do
         # Also update another product with valid data
         within row_containing_name("Bananas") do
           fill_in "Name", with: "Bananes"

From 6dc04c458c6dce5624e3af672555ac2a042f7468 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 30 Nov 2023 15:26:58 +1100
Subject: [PATCH 04/14] Show saving success message in flash

I don't know if this is the best way to do it with SR, but it works..
---
 app/reflexes/application_reflex.rb             |  7 +++++++
 app/reflexes/products_reflex.rb                |  4 ++--
 config/locales/en.yml                          |  4 ++--
 spec/reflexes/products_reflex_spec.rb          | 12 ++++++++++++
 spec/system/admin/products_v3/products_spec.rb |  6 +-----
 5 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/app/reflexes/application_reflex.rb b/app/reflexes/application_reflex.rb
index 42981e5361..a19b03b543 100644
--- a/app/reflexes/application_reflex.rb
+++ b/app/reflexes/application_reflex.rb
@@ -24,4 +24,11 @@ class ApplicationReflex < StimulusReflex::Reflex
   def morph_admin_flashes
     morph "#flashes", render(partial: "admin/shared/flashes", locals: { flashes: flash })
   end
+
+  def broadcast_admin_flashes
+    cable_ready.replace(
+      selector: "#flashes",
+      html: render(partial: "admin/shared/flashes", locals: { flashes: flash })
+    ).broadcast
+  end
 end
diff --git a/app/reflexes/products_reflex.rb b/app/reflexes/products_reflex.rb
index c75eddce24..8d2343f4f0 100644
--- a/app/reflexes/products_reflex.rb
+++ b/app/reflexes/products_reflex.rb
@@ -38,8 +38,8 @@ class ProductsReflex < ApplicationReflex
     @products = product_set.collection # use instance variable mainly for testing
 
     if product_set.save
-      # flash[:success] = with_locale { I18n.t('.success') }
-      # morph_admin_flashes  # ERROR: selector morph type has already been set
+      flash[:success] = I18n.t('admin.products_v3.bulk_update.success')
+      broadcast_admin_flashes
     elsif product_set.errors.present?
       @error_counts = { saved: product_set.saved_count, invalid: product_set.invalid.count }
     end
diff --git a/config/locales/en.yml b/config/locales/en.yml
index f512b2b9b3..dddb2f403c 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -851,8 +851,8 @@ en:
             other: "%{count} products could not be saved. Please review the errors and try again."
         save: Save changes
         reset: Discard changes
-      bulk_update: # TODO: fix these
-        success: "Products successfully updated" #TODO: add count
+      bulk_update:
+        success: Changes saved
     product_import:
       title: Product Import
       file_not_found: File not found or could not be opened
diff --git a/spec/reflexes/products_reflex_spec.rb b/spec/reflexes/products_reflex_spec.rb
index debfcd24c8..a79b32b326 100644
--- a/spec/reflexes/products_reflex_spec.rb
+++ b/spec/reflexes/products_reflex_spec.rb
@@ -7,6 +7,12 @@ describe ProductsReflex, type: :reflex, feature: :admin_style_v3 do
   let(:context) {
     { url: admin_products_url, connection: { current_user: } }
   }
+  let(:flash) { {} }
+
+  before do
+    # Mock flash, because stimulus_reflex_testing doesn't support sessions
+    allow_any_instance_of(described_class).to receive(:flash).and_return(flash)
+  end
 
   describe '#fetch' do
     subject{ build_reflex(method_name: :fetch, **context) }
@@ -54,6 +60,8 @@ describe ProductsReflex, type: :reflex, feature: :admin_style_v3 do
         product_a.reload
       }.to change{ product_a.name }.to("Pommes")
         .and change{ product_a.sku }.to("POM-00")
+
+      expect(flash).to include success: "Changes saved"
     end
 
     it "saves valid changes to products and nested variants" do
@@ -89,6 +97,8 @@ describe ProductsReflex, type: :reflex, feature: :admin_style_v3 do
         .and change{ variant_a1.sku }.to("POM-01")
         .and change{ variant_a1.price }.to(10.25)
         .and change{ variant_a1.on_hand }.to(6)
+
+      expect(flash).to include success: "Changes saved"
     end
 
     describe "sorting" do
@@ -112,6 +122,7 @@ describe ProductsReflex, type: :reflex, feature: :admin_style_v3 do
           product_a.id,
           product_b.id,
         ]
+        expect(flash).to include success: "Changes saved"
       end
     end
 
@@ -136,6 +147,7 @@ describe ProductsReflex, type: :reflex, feature: :admin_style_v3 do
 
         reflex = run_reflex(:bulk_update, params:)
         expect(reflex.get(:error_counts)).to eq({ saved: 1, invalid: 2 })
+        expect(flash).to_not include success: "Changes saved"
 
         # # WTF
         # expect{ reflex(:bulk_update, params:) }.to broadcast(
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 5e4537875c..71c5eea325 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -259,7 +259,6 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
         expect(page).to have_css "button[aria-label='On Hand']", text: "On demand"
       end
 
-      pending
       expect(page).to have_content "Changes saved"
     end
 
@@ -317,9 +316,7 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
           product_a.reload
         }.to_not change { product_a.name }
 
-        # pending("unchanged rows are being saved") # TODO: don't report unchanged rows
-        # expect(page).to_not have_content("rows were saved correctly")
-        # Both the product and variant couldn't be saved.
+        expect(page).to have_content("1 product was saved correctly")
         expect(page).to have_content("1 product could not be saved")
         expect(page).to have_content "Please review the errors and try again"
 
@@ -353,7 +350,6 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
         }.to change { product_a.name }.to("Pommes")
           .and change{ product_a.sku }.to("POM-00")
 
-        pending
         expect(page).to have_content "Changes saved"
       end
     end

From 54d61cd24d8a81dd105d9c049d44dcd6d1b3d397 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 1 Dec 2023 12:40:30 +1100
Subject: [PATCH 05/14] Don't show flashes until everything else is done.

Mario reported that it was showing before the loading spinner had gone.
Technically the loading spinner doesn't hide until after the reflex is finished, but hopefully this is close enough.
---
 app/reflexes/products_reflex.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/reflexes/products_reflex.rb b/app/reflexes/products_reflex.rb
index 8d2343f4f0..c040f76f69 100644
--- a/app/reflexes/products_reflex.rb
+++ b/app/reflexes/products_reflex.rb
@@ -39,12 +39,12 @@ class ProductsReflex < ApplicationReflex
 
     if product_set.save
       flash[:success] = I18n.t('admin.products_v3.bulk_update.success')
-      broadcast_admin_flashes
     elsif product_set.errors.present?
       @error_counts = { saved: product_set.saved_count, invalid: product_set.invalid.count }
     end
 
     render_products_form
+    broadcast_admin_flashes if flash.any?
   end
 
   private

From f1a407c8a91cb5c21a23e6eb7ee7e9215cfad80b Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 1 Dec 2023 12:33:54 +1100
Subject: [PATCH 06/14] Remove unnecessary mixin, and use generic variable for
 shadows.

---
 app/components/vertical_ellipsis_menu/component.scss  | 2 +-
 app/webpacker/css/admin_v3/components/navigation.scss | 2 +-
 app/webpacker/css/admin_v3/components/pagination.scss | 4 ++--
 app/webpacker/css/admin_v3/globals/variables.scss     | 2 +-
 app/webpacker/css/admin_v3/mixins.scss                | 4 ----
 app/webpacker/css/admin_v3/shared/forms.scss          | 2 +-
 6 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/app/components/vertical_ellipsis_menu/component.scss b/app/components/vertical_ellipsis_menu/component.scss
index 9434a94bc4..03b5cb39d8 100644
--- a/app/components/vertical_ellipsis_menu/component.scss
+++ b/app/components/vertical_ellipsis_menu/component.scss
@@ -20,7 +20,7 @@
     padding-top: 5px;
     padding-bottom: 5px;
     background-color: white;
-    @include defaultBoxShadow;
+    box-shadow: $box-shadow;
     border-radius: 3px;
     min-width: 80px;
     display: none;
diff --git a/app/webpacker/css/admin_v3/components/navigation.scss b/app/webpacker/css/admin_v3/components/navigation.scss
index e5d448f2d4..3b2c568646 100644
--- a/app/webpacker/css/admin_v3/components/navigation.scss
+++ b/app/webpacker/css/admin_v3/components/navigation.scss
@@ -61,7 +61,7 @@ nav.menu {
 }
 
 #admin-menu {
-  @include defaultBoxShadow;
+  box-shadow: $box-shadow;
 
   li {
     .dropdown {
diff --git a/app/webpacker/css/admin_v3/components/pagination.scss b/app/webpacker/css/admin_v3/components/pagination.scss
index 82642f54a6..98df96f06e 100644
--- a/app/webpacker/css/admin_v3/components/pagination.scss
+++ b/app/webpacker/css/admin_v3/components/pagination.scss
@@ -20,7 +20,7 @@
     display: inline-block;
     text-align: center;
     background-color: $color-1;
-    @include defaultBoxShadow;
+    box-shadow: $box-shadow;
     border-radius: 4px;
     border: none;
     color: $color-8;
@@ -77,7 +77,7 @@
 
     background-color: $white;
     color: $near-black;
-    box-shadow: $color-btn-shadow;
+    box-shadow: $box-shadow;
 
     &.active {
       color: $white;
diff --git a/app/webpacker/css/admin_v3/globals/variables.scss b/app/webpacker/css/admin_v3/globals/variables.scss
index 7eecf26ea8..7a682d895d 100644
--- a/app/webpacker/css/admin_v3/globals/variables.scss
+++ b/app/webpacker/css/admin_v3/globals/variables.scss
@@ -40,7 +40,6 @@ $padding-tbl-cell-relaxed:       12px 12px;
 // Button colors
 $color-btn-bg:                   $teal !default;
 $color-btn-text:                 $white !default;
-$color-btn-shadow:               0px 1px 0px rgba(0, 0, 0, 0.05), 0px 2px 2px rgba(0, 0, 0, 0.07) !default;
 $color-btn-hover-bg:             $orient !default;
 $color-btn-hover-text:           $white !default;
 $color-btn-hover-border:         $dark-blue !default;
@@ -161,6 +160,7 @@ $h2-size:                        $h3-size + 2 !default;
 $h1-size:                        $h2-size + 2 !default;
 
 $border-radius:                  4px !default;
+$box-shadow:                     0px 1px 0px rgba(0, 0, 0, 0.05), 0px 2px 2px rgba(0, 0, 0, 0.07) !default;
 
 $font-weight-bold:               600 !default;
 $font-weight-normal:             400 !default;
diff --git a/app/webpacker/css/admin_v3/mixins.scss b/app/webpacker/css/admin_v3/mixins.scss
index 5301558d32..c2471a52ba 100644
--- a/app/webpacker/css/admin_v3/mixins.scss
+++ b/app/webpacker/css/admin_v3/mixins.scss
@@ -1,7 +1,3 @@
-@mixin defaultBoxShadow {
-  box-shadow: 0px 1px 0px rgba(0, 0, 0, 0.05), 0px 2px 2px rgba(0, 0, 0, 0.07);
-}
-
 @mixin arrowDown {
   content: " ";
   display: block;
diff --git a/app/webpacker/css/admin_v3/shared/forms.scss b/app/webpacker/css/admin_v3/shared/forms.scss
index 8a37545a29..601fad9bde 100644
--- a/app/webpacker/css/admin_v3/shared/forms.scss
+++ b/app/webpacker/css/admin_v3/shared/forms.scss
@@ -266,7 +266,7 @@ fieldset {
 }
 
 .form-actions {
-  @include defaultBoxShadow;
+  box-shadow: $box-shadow;
   background-color: $fair-pink;
   border: none;
   border-left: 4px solid $red;

From 44187658be8d0ed8a239e5d2d64d0f34f315fe6c Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 30 Nov 2023 16:13:45 +1100
Subject: [PATCH 07/14] Copy messages styles for admin_v3

---
 app/webpacker/css/admin_v3/all.scss           |  2 +-
 .../css/admin_v3/components/messages.scss     | 72 +++++++++++++++++++
 2 files changed, 73 insertions(+), 1 deletion(-)
 create mode 100644 app/webpacker/css/admin_v3/components/messages.scss

diff --git a/app/webpacker/css/admin_v3/all.scss b/app/webpacker/css/admin_v3/all.scss
index 3ee85908f9..6e4fe6b7bb 100644
--- a/app/webpacker/css/admin_v3/all.scss
+++ b/app/webpacker/css/admin_v3/all.scss
@@ -56,7 +56,7 @@
 @import "../admin/components/dialogs";
 @import "../admin/components/input";
 @import "../admin/components/jquery_dialog";
-@import "../admin/components/messages";
+@import "components/messages"; // admin_v3
 @import "components/navigation"; // admin_v3
 @import "../admin/components/ng-cloak";
 @import "../admin/components/page_actions";
diff --git a/app/webpacker/css/admin_v3/components/messages.scss b/app/webpacker/css/admin_v3/components/messages.scss
new file mode 100644
index 0000000000..61dd751a55
--- /dev/null
+++ b/app/webpacker/css/admin_v3/components/messages.scss
@@ -0,0 +1,72 @@
+.errorExplanation {
+  padding: 10px;
+  border: 1px solid very-light($color-error, 12);
+  background-color: very-light($color-error, 6);
+  border-radius: 3px;
+  color: $color-error;
+  margin-bottom: 15px;
+
+  h2 {
+    font-size: 140%;
+    color: $color-error;
+    margin-bottom: 5px;
+  }
+
+  p {
+    padding: 10px 0;
+  }
+
+  ul {
+    list-style-position: inside;
+
+    li {
+      font-weight: $font-weight-bold;
+    }
+  }
+}
+
+.flash-container {
+  position: fixed;
+  top: 0;
+  left: 0;
+  width: 100%;
+  z-index: 1000;
+
+  .flash {
+    padding: 18px;
+    text-align: center;
+    font-size: 120%;
+    color: $color-1;
+    font-weight: 600;
+    margin-top: 0;
+
+    &.notice {
+      background-color: rgba($color-notice, 0.8);
+    }
+    &.success {
+      background-color: rgba($color-success, 0.8);
+    }
+    &.error {
+      background-color: rgba($color-error, 0.8);
+    }
+
+    // Adjust heights to fit main layout dimension (header, navbar...)
+    &:nth-child(2) {
+      padding: 24px;
+    }
+    &:nth-child(3) {
+      padding: 20px;
+    }
+  }
+}
+
+.notice:not(.flash) {
+  padding: 1rem;
+  margin-bottom: 1.5rem;
+  background-color: $spree-light-blue;
+  border-radius: $border-radius;
+
+  a {
+    font-weight: bold;
+  }
+}

From 20afe5b99c7ceed118b6a04b439f203c1e370937 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 30 Nov 2023 16:15:49 +1100
Subject: [PATCH 08/14] Update style of flash messages

They now hover near the bottom of the screen.

I've created new variables so as not to mess with the existing use of color-success etc.
---
 .../css/admin_v3/components/messages.scss     | 40 +++++++++----------
 .../css/admin_v3/globals/variables.scss       |  6 +++
 app/webpacker/css/admin_v3/shared/forms.scss  |  4 +-
 3 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/app/webpacker/css/admin_v3/components/messages.scss b/app/webpacker/css/admin_v3/components/messages.scss
index 61dd751a55..a2f9b8d562 100644
--- a/app/webpacker/css/admin_v3/components/messages.scss
+++ b/app/webpacker/css/admin_v3/components/messages.scss
@@ -27,35 +27,35 @@
 
 .flash-container {
   position: fixed;
-  top: 0;
-  left: 0;
+  bottom: 0;
   width: 100%;
   z-index: 1000;
+  display: flex;
+  justify-content: center;
 
   .flash {
-    padding: 18px;
-    text-align: center;
-    font-size: 120%;
-    color: $color-1;
+    position: relative;
+    bottom: 3.5rem;
+    padding: 1rem;
+    min-width: 24rem;
+    max-width: 48.25em;
     font-weight: 600;
-    margin-top: 0;
+    background-color: $light-grey;
+    border-radius: $border-radius;
+    box-shadow: $box-shadow;
 
-    &.notice {
-      background-color: rgba($color-notice, 0.8);
-    }
     &.success {
-      background-color: rgba($color-success, 0.8);
+      color: $color-flash-success-text;
+      background-color: $color-flash-success-bg;
+    }
+    &.notice {
+      color: $color-flash-notice-text;
+      background-color: $color-flash-notice-bg;
+      border-left: $border-radius solid $red;
     }
     &.error {
-      background-color: rgba($color-error, 0.8);
-    }
-
-    // Adjust heights to fit main layout dimension (header, navbar...)
-    &:nth-child(2) {
-      padding: 24px;
-    }
-    &:nth-child(3) {
-      padding: 20px;
+      color: $color-flash-error-text;
+      background-color: $color-flash-error-bg;
     }
   }
 }
diff --git a/app/webpacker/css/admin_v3/globals/variables.scss b/app/webpacker/css/admin_v3/globals/variables.scss
index 7a682d895d..d2da9c519c 100644
--- a/app/webpacker/css/admin_v3/globals/variables.scss
+++ b/app/webpacker/css/admin_v3/globals/variables.scss
@@ -25,6 +25,12 @@ $color-success:                  $green !default;
 $color-notice:                   $yellow !default;
 $color-warning:                  $red !default;
 $color-error:                    $red !default;
+$color-flash-success-text:       $white !default;
+$color-flash-success-bg:         $near-black !default;
+$color-flash-notice-text:        $color-body-text !default;
+$color-flash-notice-bg:          $fair-pink !default;
+$color-flash-error-text:         $white !default;
+$color-flash-error-bg:           $color-error !default;
 
 // Table styles
 $color-tbl-bg:                   $light-grey !default;
diff --git a/app/webpacker/css/admin_v3/shared/forms.scss b/app/webpacker/css/admin_v3/shared/forms.scss
index 601fad9bde..90ae20cf3f 100644
--- a/app/webpacker/css/admin_v3/shared/forms.scss
+++ b/app/webpacker/css/admin_v3/shared/forms.scss
@@ -269,8 +269,8 @@ fieldset {
   box-shadow: $box-shadow;
   background-color: $fair-pink;
   border: none;
-  border-left: 4px solid $red;
-  border-radius: 4px;
+  border-left: $border-radius solid $red;
+  border-radius: $border-radius;
   margin: 0.5em 0;
   padding: 0;
 

From 0b4013dd11c14ecbea13348ab2331bd3fae0cef4 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 30 Nov 2023 17:08:07 +1100
Subject: [PATCH 09/14] Add dismiss button to flashes

And updating the secondary button style to ensure it's always white background
---
 app/views/admin/shared/_flashes.html.haml           | 4 +++-
 app/webpacker/css/admin/components/messages.scss    | 4 ++++
 app/webpacker/css/admin_v3/components/buttons.scss  | 6 +++---
 app/webpacker/css/admin_v3/components/messages.scss | 9 ++++++++-
 app/webpacker/css/admin_v3/globals/variables.scss   | 2 ++
 config/locales/en.yml                               | 2 ++
 spec/support/request/web_helper.rb                  | 2 +-
 7 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/app/views/admin/shared/_flashes.html.haml b/app/views/admin/shared/_flashes.html.haml
index 1683d5a1d8..0caad6c2b2 100644
--- a/app/views/admin/shared/_flashes.html.haml
+++ b/app/views/admin/shared/_flashes.html.haml
@@ -3,4 +3,6 @@
     - flashes.each do |type, msg|
       .animate-show{"data-controller": "flash", "data-flash-auto-close-value": "true"}
         .flash{type: "#{type}", class: "#{type}"}
-          %span= msg
+          .msg= msg
+          .actions
+            %button.secondary{"data-action": "click->flash#close"}= t('.dismiss')
diff --git a/app/webpacker/css/admin/components/messages.scss b/app/webpacker/css/admin/components/messages.scss
index 61dd751a55..f92498a599 100644
--- a/app/webpacker/css/admin/components/messages.scss
+++ b/app/webpacker/css/admin/components/messages.scss
@@ -57,6 +57,10 @@
     &:nth-child(3) {
       padding: 20px;
     }
+
+    .actions {
+      display: none; /* avoid adding new button on old design */
+    }
   }
 }
 
diff --git a/app/webpacker/css/admin_v3/components/buttons.scss b/app/webpacker/css/admin_v3/components/buttons.scss
index 13ecab6d7b..a5ab9c3b7f 100644
--- a/app/webpacker/css/admin_v3/components/buttons.scss
+++ b/app/webpacker/css/admin_v3/components/buttons.scss
@@ -53,14 +53,14 @@ button:not(.plain):not(.trix-button),
   }
 
   &.secondary {
-    background-color: transparent;
+    background-color: $color-btn-secondary-bg;
     border: 1px solid $color-btn-bg;
     color: $color-btn-bg;
 
     &:hover {
       background-color: $color-11;
-      border: 1px solid $color-btn-hover-bg;
-      color: $color-btn-hover-bg;
+      border: 1px solid $color-btn-secondary-hover-bg;
+      color: $color-btn-secondary-hover-bg;
     }
 
     &:active,
diff --git a/app/webpacker/css/admin_v3/components/messages.scss b/app/webpacker/css/admin_v3/components/messages.scss
index a2f9b8d562..4a956cc831 100644
--- a/app/webpacker/css/admin_v3/components/messages.scss
+++ b/app/webpacker/css/admin_v3/components/messages.scss
@@ -35,8 +35,10 @@
 
   .flash {
     position: relative;
+    display: flex;
+    align-items: center;
     bottom: 3.5rem;
-    padding: 1rem;
+    padding: 0.25rem;
     min-width: 24rem;
     max-width: 48.25em;
     font-weight: 600;
@@ -57,6 +59,11 @@
       color: $color-flash-error-text;
       background-color: $color-flash-error-bg;
     }
+
+    .msg {
+      flex-grow: 1;
+      margin: 0.25rem 0.75rem;
+    }
   }
 }
 
diff --git a/app/webpacker/css/admin_v3/globals/variables.scss b/app/webpacker/css/admin_v3/globals/variables.scss
index d2da9c519c..e41f2a9597 100644
--- a/app/webpacker/css/admin_v3/globals/variables.scss
+++ b/app/webpacker/css/admin_v3/globals/variables.scss
@@ -53,6 +53,8 @@ $color-btn-disabled-bg:          $medium-grey !default;
 $color-btn-disabled-text:        $lighter-grey !default;
 $color-btn-red-bg:               $red !default;
 $color-btn-red-hover-bg:         $roof-terracotta !default;
+$color-btn-secondary-bg:         $white !default;
+$color-btn-secondary-hover-bg:   $orient !default;
 
 // Actions colors
 $color-action-edit-bg:           very-light($teal) !default;
diff --git a/config/locales/en.yml b/config/locales/en.yml
index dddb2f403c..9db09b06a2 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1502,6 +1502,8 @@ en:
         has_no_payment_methods: "%{enterprise} has no payment methods"
         has_no_shipping_methods: "%{enterprise} has no shipping methods"
         has_no_enterprise_fees: "%{enterprise} has no enterprise fees"
+      flashes:
+        dismiss: Dismiss
       side_menu:
         enterprise:
           primary_details: "Primary Details"
diff --git a/spec/support/request/web_helper.rb b/spec/support/request/web_helper.rb
index 10efa75693..4e8aad2be0 100644
--- a/spec/support/request/web_helper.rb
+++ b/spec/support/request/web_helper.rb
@@ -19,7 +19,7 @@ module WebHelper
   end
 
   def flash_message
-    find('.flash', visible: false).text(:all).strip
+    find('.flash .msg', visible: false).text(:all).strip
   end
 
   def handle_js_confirm(accept = true)

From fbf0afa15dd243c7bcf6d698d791bdc35d02b562 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 1 Dec 2023 14:49:26 +1100
Subject: [PATCH 10/14] Auto dismiss success flashes only

---
 app/views/admin/shared/_flashes.html.haml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/views/admin/shared/_flashes.html.haml b/app/views/admin/shared/_flashes.html.haml
index 0caad6c2b2..6a1128d330 100644
--- a/app/views/admin/shared/_flashes.html.haml
+++ b/app/views/admin/shared/_flashes.html.haml
@@ -1,7 +1,7 @@
 #flashes
   - if defined? flashes
     - flashes.each do |type, msg|
-      .animate-show{"data-controller": "flash", "data-flash-auto-close-value": "true"}
+      .animate-show{"data-controller": "flash", "data-flash-auto-close-value": type == 'success'}
         .flash{type: "#{type}", class: "#{type}"}
           .msg= msg
           .actions

From 32a4088386ebe54b7beec2e09c9372bc5ca12b7d Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 1 Dec 2023 15:25:50 +1100
Subject: [PATCH 11/14] Revert "#11067, remove messages to match with old UI
 UX"

Now we support flash messages, we can show it!

This reverts commit d8904099dd85bcf358c2e7182349c81325faa755.
---
 app/controllers/spree/admin/products_controller.rb | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/app/controllers/spree/admin/products_controller.rb b/app/controllers/spree/admin/products_controller.rb
index 3ecc96dab6..5b5b5c1d9d 100644
--- a/app/controllers/spree/admin/products_controller.rb
+++ b/app/controllers/spree/admin/products_controller.rb
@@ -82,7 +82,12 @@ module Spree
 
       def clone
         @new = @product.duplicate
-        @new.save
+
+        flash[:success] = if @new.save
+                            Spree.t('notice_messages.product_cloned')
+                          else
+                            Spree.t('notice_messages.product_not_cloned')
+                          end
 
         redirect_to spree.admin_products_url
       end

From 7fe2284d84d6e18dce430dc98f40e8d0ec2fcad1 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 6 Dec 2023 09:57:24 +1100
Subject: [PATCH 12/14] Refactor: Move ID out of partial

Because it can only be used once. But the classname can be used each time the partial is included.
---
 app/views/admin/shared/_flashes.html.haml     | 2 +-
 app/views/spree/layouts/_admin_body.html.haml | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/views/admin/shared/_flashes.html.haml b/app/views/admin/shared/_flashes.html.haml
index 6a1128d330..4e8b94dace 100644
--- a/app/views/admin/shared/_flashes.html.haml
+++ b/app/views/admin/shared/_flashes.html.haml
@@ -1,4 +1,4 @@
-#flashes
+.flash-container
   - if defined? flashes
     - flashes.each do |type, msg|
       .animate-show{"data-controller": "flash", "data-flash-auto-close-value": type == 'success'}
diff --git a/app/views/spree/layouts/_admin_body.html.haml b/app/views/spree/layouts/_admin_body.html.haml
index 4ca442a850..76f1f77e26 100644
--- a/app/views/spree/layouts/_admin_body.html.haml
+++ b/app/views/spree/layouts/_admin_body.html.haml
@@ -3,7 +3,7 @@
 = yield :stripe_js
 
 #wrapper
-  .flash-container
+  #flashes
     = render partial: "admin/shared/flashes", locals: { flashes: flash }
 
   = render partial: "spree/layouts/admin/progress_spinner"

From 0f46da07b2bca7e8d0fd248b0e474216e9127056 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 6 Dec 2023 09:58:50 +1100
Subject: [PATCH 13/14] Render flashes along with table

It doesn't matter where the flash messages appear in the HTML (thanks to fixed positioning), so why not keep it simple and send them with the main response.

preventDefault in case we are inside a form, so the button doesn't submit it.
---
 app/reflexes/application_reflex.rb                  | 7 -------
 app/reflexes/products_reflex.rb                     | 6 +++---
 app/views/admin/products_v3/_table.html.haml        | 1 +
 app/webpacker/controllers/flash_controller.js       | 3 ++-
 app/webpacker/css/admin_v3/components/messages.scss | 1 +
 5 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/app/reflexes/application_reflex.rb b/app/reflexes/application_reflex.rb
index a19b03b543..42981e5361 100644
--- a/app/reflexes/application_reflex.rb
+++ b/app/reflexes/application_reflex.rb
@@ -24,11 +24,4 @@ class ApplicationReflex < StimulusReflex::Reflex
   def morph_admin_flashes
     morph "#flashes", render(partial: "admin/shared/flashes", locals: { flashes: flash })
   end
-
-  def broadcast_admin_flashes
-    cable_ready.replace(
-      selector: "#flashes",
-      html: render(partial: "admin/shared/flashes", locals: { flashes: flash })
-    ).broadcast
-  end
 end
diff --git a/app/reflexes/products_reflex.rb b/app/reflexes/products_reflex.rb
index c040f76f69..4154a7b61f 100644
--- a/app/reflexes/products_reflex.rb
+++ b/app/reflexes/products_reflex.rb
@@ -43,8 +43,7 @@ class ProductsReflex < ApplicationReflex
       @error_counts = { saved: product_set.saved_count, invalid: product_set.invalid.count }
     end
 
-    render_products_form
-    broadcast_admin_flashes if flash.any?
+    render_products_form_with_flash
   end
 
   private
@@ -85,9 +84,10 @@ class ProductsReflex < ApplicationReflex
     morph :nothing
   end
 
-  def render_products_form
+  def render_products_form_with_flash
     locals = { products: @products }
     locals[:error_counts] = @error_counts if @error_counts.present?
+    locals[:flashes] = flash if flash.any?
 
     cable_ready.replace(
       selector: "#products-form",
diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 519738169c..0462529857 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -4,6 +4,7 @@
                    'data-controller': "bulk-form", 'data-bulk-form-disable-selector-value': "#sort,#filters",
                    'data-bulk-form-error-value': defined?(error_counts),
                   } do |form|
+  = render(partial: "admin/shared/flashes", locals: { flashes: }) if defined? flashes
   %fieldset.form-actions{ class: ("hidden" unless defined?(error_counts)), 'data-bulk-form-target': "actions" }
     .container
       .status.eleven.columns
diff --git a/app/webpacker/controllers/flash_controller.js b/app/webpacker/controllers/flash_controller.js
index 100d34fa69..84dfe7eaf9 100644
--- a/app/webpacker/controllers/flash_controller.js
+++ b/app/webpacker/controllers/flash_controller.js
@@ -15,11 +15,12 @@ export default class extends Controller {
     }
   }
 
-  close() {
+  close(e) {
     // Fade out
     this.element.classList.remove("animate-show");
     this.element.classList.add("animate-hide-500");
     setTimeout(this.remove.bind(this), 500);
+    e && e.preventDefault(); // Prevent submitting if we're inside a form
   }
 
   remove() {
diff --git a/app/webpacker/css/admin_v3/components/messages.scss b/app/webpacker/css/admin_v3/components/messages.scss
index 4a956cc831..1d574a9a07 100644
--- a/app/webpacker/css/admin_v3/components/messages.scss
+++ b/app/webpacker/css/admin_v3/components/messages.scss
@@ -27,6 +27,7 @@
 
 .flash-container {
   position: fixed;
+  left: 0;
   bottom: 0;
   width: 100%;
   z-index: 1000;

From 0f3a952fd00c5cec66f59c45de783a6b9de00ccc Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 6 Dec 2023 15:07:37 +1100
Subject: [PATCH 14/14] Add translation for product cloned message

A validation error shouldn't happen. If it does, it's an exception, not an error.
---
 app/controllers/spree/admin/products_controller.rb | 8 ++------
 config/locales/en.yml                              | 2 ++
 spec/system/admin/products_v3/products_spec.rb     | 3 ++-
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/app/controllers/spree/admin/products_controller.rb b/app/controllers/spree/admin/products_controller.rb
index 5b5b5c1d9d..e153faff38 100644
--- a/app/controllers/spree/admin/products_controller.rb
+++ b/app/controllers/spree/admin/products_controller.rb
@@ -82,13 +82,9 @@ module Spree
 
       def clone
         @new = @product.duplicate
+        raise "Clone failed" unless @new.save
 
-        flash[:success] = if @new.save
-                            Spree.t('notice_messages.product_cloned')
-                          else
-                            Spree.t('notice_messages.product_not_cloned')
-                          end
-
+        flash[:success] = t('.success')
         redirect_to spree.admin_products_url
       end
 
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 9db09b06a2..fc03585bc0 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -4348,6 +4348,8 @@ See the %{link} to find out more about %{sitename}'s features and to start using
           bulk_unit_size: Bulk unit size
         display_as:
           display_as: Display As
+        clone:
+          success: Product cloned
       reports:
         table:
           select_and_search: "Select filters and click on %{option} to access your data."
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 71c5eea325..c34e40b934 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -237,7 +237,6 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
         expect(page).to have_css "button[aria-label='On Hand']", text: "6"
       end
 
-      pending
       expect(page).to have_content "Changes saved"
     end
 
@@ -398,6 +397,8 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
           click_link('Clone')
         end
 
+        expect(page).to have_content "Product cloned"
+
         within "table.products" do
           # Gather input values, because page.content doesn't include them.
           input_content = page.find_all('input[type=text]').map(&:value).join