From a6a3a02d79ea318f54c850ef5427cd57e0de09ec Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 10 Jan 2024 15:54:43 +1100
Subject: [PATCH 01/13] Use padding to set button size

It's more flexible.
Also reduced the overall size to suit the desired table row sizes.
---
 app/components/vertical_ellipsis_menu/component.scss | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/app/components/vertical_ellipsis_menu/component.scss b/app/components/vertical_ellipsis_menu/component.scss
index 03b5cb39d8..9e8bc9d878 100644
--- a/app/components/vertical_ellipsis_menu/component.scss
+++ b/app/components/vertical_ellipsis_menu/component.scss
@@ -1,16 +1,13 @@
 .vertical-ellipsis-menu {
   position: relative;
-  width: $btn-relaxed-height;
 
   i.fa-ellipsis-v {
     cursor: pointer;
     display: block;
-    height: $btn-relaxed-height;
-    width: $btn-relaxed-height;
-    line-height: $btn-relaxed-height;
     text-align: center;
     border-radius: 3px;
     background-color: white;
+    padding: 10px 14px;
   }
 
   .vertical-ellipsis-menu-content {

From ca73a9ab62529bcf2fa6c5ad6ec4294329e165ea Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 10 Jan 2024 15:55:40 +1100
Subject: [PATCH 02/13] Reduce space used by image

---
 app/webpacker/css/admin/products_v3.scss | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index 4d6450e23f..c03f8ee016 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -131,6 +131,10 @@
 
       td {
         border-bottom: 2px solid $color-tbl-bg;
+
+        &.with-image {
+          padding: 8px;
+        }
       }
 
       tr:first-child td {
@@ -379,6 +383,7 @@
     position: relative;
 
     img {
+      display: block;
       border-radius: $border-radius;
     }
 

From c584d639cc1c6d656c81cd8a917cc3aa4513b590 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 10 Jan 2024 15:55:56 +1100
Subject: [PATCH 03/13] Show default image if none set

---
 app/views/admin/products_v3/_table.html.haml | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index dcdec95520..68dfa43cff 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -48,10 +48,10 @@
       = form.fields_for("products", product, index: product_index) do |product_form|
         %tbody.relaxed{ 'data-record-id': product_form.object.id }
           %tr
-            %td
-              - if product.image.present?
-                .image-field
-                  = image_tag product.image.url(:mini), width: 40
+            %td.with-image
+              .image-field
+                = image_tag product.image&.url(:mini) || Spree::Image.default_image_url(:mini), width: 40, height: 40
+                - if product.image.present?
                   = link_to t('admin.products_page.image.edit'), edit_admin_product_image_path(product, product.image), class: "button secondary mini"
             %td.field.align-left.header
               = product_form.hidden_field :id

From 91abb856a628419a3d420c14eb397687ebbb0718 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 11 Jan 2024 12:07:50 +1100
Subject: [PATCH 04/13] DRY

Refactor to use the (previously unused) shared method.
---
 app/controllers/spree/admin/images_controller.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/controllers/spree/admin/images_controller.rb b/app/controllers/spree/admin/images_controller.rb
index 4fed1aba31..89e2087c0b 100644
--- a/app/controllers/spree/admin/images_controller.rb
+++ b/app/controllers/spree/admin/images_controller.rb
@@ -32,7 +32,7 @@ module Spree
 
         if @object.save
           flash[:success] = flash_message_for(@object, :successfully_created)
-          redirect_to spree.admin_product_images_url(params[:product_id], @url_filters)
+          redirect_to location_after_save
         else
           respond_with(@object)
         end
@@ -44,7 +44,7 @@ module Spree
 
         if @object.update(permitted_resource_params)
           flash[:success] = flash_message_for(@object, :successfully_updated)
-          redirect_to spree.admin_product_images_url(params[:product_id], @url_filters)
+          redirect_to location_after_save
         else
           respond_with(@object)
         end
@@ -58,7 +58,7 @@ module Spree
           flash[:success] = flash_message_for(@object, :successfully_removed)
         end
 
-        redirect_to spree.admin_product_images_url(params[:product_id], @url_filters)
+        redirect_to location_after_save
       end
 
       private
@@ -76,7 +76,7 @@ module Spree
       end
 
       def location_after_save
-        spree.admin_product_images_url(@product)
+        spree.admin_product_images_url(params[:product_id], @url_filters)
       end
 
       def load_data

From 137489f492e92b34fe8ad934682dc6bd0f496f0c Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 11 Jan 2024 13:33:06 +1100
Subject: [PATCH 05/13] Refactor: move basic modal styles to generic parent
 component

The style was already being shared with a sibling component.

Now we can instantiate a plain old 'modal'.
---
 .../confirm_modal_component.html.haml                     | 2 +-
 .../help_modal_component/help_modal_component.html.haml   | 2 +-
 app/components/modal_component/modal_component.html.haml  | 8 ++++++++
 .../modal_component.scss}                                 | 3 ++-
 .../spree/admin/orders/bulk/_invoice_modal.html.haml      | 2 +-
 app/webpacker/css/admin/all.scss                          | 2 +-
 app/webpacker/css/admin_v3/all.scss                       | 2 +-
 app/webpacker/css/darkswarm/all.scss                      | 2 +-
 spec/javascripts/stimulus/help_modal_controller_test.js   | 2 +-
 spec/system/admin/enterprise_roles_spec.rb                | 2 +-
 10 files changed, 18 insertions(+), 9 deletions(-)
 create mode 100644 app/components/modal_component/modal_component.html.haml
 rename app/components/{help_modal_component/help_modal_component.scss => modal_component/modal_component.scss} (69%)

diff --git a/app/components/confirm_modal_component/confirm_modal_component.html.haml b/app/components/confirm_modal_component/confirm_modal_component.html.haml
index 5fe1d1300b..1184602a67 100644
--- a/app/components/confirm_modal_component/confirm_modal_component.html.haml
+++ b/app/components/confirm_modal_component/confirm_modal_component.html.haml
@@ -1,6 +1,6 @@
 %div{ id: @id, "data-controller": "modal #{@controller}", "data-action": "keyup@document->modal#closeIfEscapeKey", "data-#{@controller}-reflex-value": @reflex }
   .reveal-modal-bg.fade{ "data-modal-target": "background", "data-action": "click->modal#close" }
-  .reveal-modal.fade.tiny.help-modal{ "data-modal-target": "modal" }
+  .reveal-modal.fade.tiny.modal-component{ "data-modal-target": "modal" }
     = content
 
     = render @message if @message
diff --git a/app/components/help_modal_component/help_modal_component.html.haml b/app/components/help_modal_component/help_modal_component.html.haml
index af7d602724..adc6c54822 100644
--- a/app/components/help_modal_component/help_modal_component.html.haml
+++ b/app/components/help_modal_component/help_modal_component.html.haml
@@ -1,6 +1,6 @@
 %div{ id: @id, "data-controller": "help-modal", "data-action": "keyup@document->help-modal#closeIfEscapeKey" }
   .reveal-modal-bg.fade{ "data-help-modal-target": "background", "data-action": "click->help-modal#close" }
-  .reveal-modal.fade.small.help-modal{ "data-help-modal-target": "modal" }
+  .reveal-modal.fade.small.modal-component{ "data-help-modal-target": "modal" }
     = content
 
     - if close_button?
diff --git a/app/components/modal_component/modal_component.html.haml b/app/components/modal_component/modal_component.html.haml
new file mode 100644
index 0000000000..8c13b1dee0
--- /dev/null
+++ b/app/components/modal_component/modal_component.html.haml
@@ -0,0 +1,8 @@
+%div{ id: @id, "data-controller": "modal", "data-action": "keyup@document->modal#closeIfEscapeKey" }
+  .reveal-modal-bg.fade{ "data-modal-target": "background", "data-action": "click->modal#close" }
+  .reveal-modal.fade.small.modal-component{ "data-modal-target": "modal" }
+    = content
+
+    - if close_button?
+      .text-center
+        %input{ class: "button icon-plus #{close_button_class}", type: 'button', value: t('js.admin.modals.close'), "data-action": "click->modal#close" }
diff --git a/app/components/help_modal_component/help_modal_component.scss b/app/components/modal_component/modal_component.scss
similarity index 69%
rename from app/components/help_modal_component/help_modal_component.scss
rename to app/components/modal_component/modal_component.scss
index d323b47992..5223d23824 100644
--- a/app/components/help_modal_component/help_modal_component.scss
+++ b/app/components/modal_component/modal_component.scss
@@ -1,4 +1,5 @@
-.help-modal {
+// class name 'modal' is already taken by 'custom-alert' and 'custom-confirm'.
+.modal-component {
   visibility: visible;
   position: fixed;
   top: 3em;
diff --git a/app/views/spree/admin/orders/bulk/_invoice_modal.html.haml b/app/views/spree/admin/orders/bulk/_invoice_modal.html.haml
index 07c2f30121..53f7982f25 100644
--- a/app/views/spree/admin/orders/bulk/_invoice_modal.html.haml
+++ b/app/views/spree/admin/orders/bulk/_invoice_modal.html.haml
@@ -1,6 +1,6 @@
 %div{ id: "bulk_invoices_modal", "data-controller": "modal", "data-modal-instant-value": true, "data-action": "keyup@document->modal#closeIfEscapeKey" }
   .reveal-modal-bg.fade{ "data-modal-target": "background", "data-action": "click->modal#remove" }
-  .reveal-modal.fade.tiny.help-modal{ "data-modal-target": "modal" }
+  .reveal-modal.fade.tiny.modal-component{ "data-modal-target": "modal" }
     %div.fullwidth.align-center
       %h4.modal-title
         = t('js.admin.orders.index.compiling_invoices')
diff --git a/app/webpacker/css/admin/all.scss b/app/webpacker/css/admin/all.scss
index 56d522ae75..2cb175b8f7 100644
--- a/app/webpacker/css/admin/all.scss
+++ b/app/webpacker/css/admin/all.scss
@@ -124,6 +124,6 @@
 @import "~tom-select/src/scss/tom-select.default";
 @import "components/tom_select";
 
-@import "app/components/help_modal_component/help_modal_component";
+@import "app/components/modal_component/modal_component";
 @import "app/components/confirm_modal_component/confirm_modal_component";
 @import "app/webpacker/css/admin/trix.scss";
diff --git a/app/webpacker/css/admin_v3/all.scss b/app/webpacker/css/admin_v3/all.scss
index 664a4fda11..274e76a76e 100644
--- a/app/webpacker/css/admin_v3/all.scss
+++ b/app/webpacker/css/admin_v3/all.scss
@@ -128,7 +128,7 @@
 @import "~tom-select/src/scss/tom-select.default";
 @import "components/tom_select"; // admin_v3
 
-@import "app/components/help_modal_component/help_modal_component";
+@import "app/components/modal_component/modal_component";
 @import "app/components/confirm_modal_component/confirm_modal_component";
 @import "app/components/vertical_ellipsis_menu/component"; // admin_v3 and only V3
 @import "app/webpacker/css/admin/trix.scss";
diff --git a/app/webpacker/css/darkswarm/all.scss b/app/webpacker/css/darkswarm/all.scss
index a78b2c30e2..be25b548d5 100644
--- a/app/webpacker/css/darkswarm/all.scss
+++ b/app/webpacker/css/darkswarm/all.scss
@@ -77,5 +77,5 @@ ofn-modal {
 @import "../shared/question-mark-icon";
 @import '../admin/shared/scroll_bar';
 
-@import 'app/components/help_modal_component/help_modal_component';
+@import 'app/components/modal_component/modal_component';
 @import 'app/components/confirm_modal_component/confirm_modal_component';
diff --git a/spec/javascripts/stimulus/help_modal_controller_test.js b/spec/javascripts/stimulus/help_modal_controller_test.js
index 1120ede695..944cdea04c 100644
--- a/spec/javascripts/stimulus/help_modal_controller_test.js
+++ b/spec/javascripts/stimulus/help_modal_controller_test.js
@@ -34,7 +34,7 @@ describe("HelpModalController", () => {
              data-action="click->help-modal#close">
         </div>
         <div id="modal"
-             class="reveal-modal.fade.medium.help-modal"
+             class="reveal-modal.fade.medium.modal-component"
              data-help-modal-target="modal">
           Hello world
           <a id="close-link" data-action="click->help-modal#close">Close</a>
diff --git a/spec/system/admin/enterprise_roles_spec.rb b/spec/system/admin/enterprise_roles_spec.rb
index d7431592e2..c82bee932d 100644
--- a/spec/system/admin/enterprise_roles_spec.rb
+++ b/spec/system/admin/enterprise_roles_spec.rb
@@ -149,7 +149,7 @@ create(:enterprise)
       end
 
       xit "can invite unregistered users to be managers" do
-        find('a.button.help-modal').click
+        find('a.button.modal').click
         expect(page).to have_css '#invite-manager-modal'
 
         within '#invite-manager-modal' do

From 38661a67bc7c3ef35e3304230839e1a52770123d Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 11 Jan 2024 14:01:11 +1100
Subject: [PATCH 06/13] Add option to open modal instantly

Using pre-existing feature in ModalController
---
 app/components/modal_component.rb                        | 3 ++-
 app/components/modal_component/modal_component.html.haml | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/app/components/modal_component.rb b/app/components/modal_component.rb
index 70226295c7..a538eb19b1 100644
--- a/app/components/modal_component.rb
+++ b/app/components/modal_component.rb
@@ -1,9 +1,10 @@
 # frozen_string_literal: true
 
 class ModalComponent < ViewComponent::Base
-  def initialize(id:, close_button: true)
+  def initialize(id:, close_button: true, instant: false)
     @id = id
     @close_button = close_button
+    @instant = instant
   end
 
   private
diff --git a/app/components/modal_component/modal_component.html.haml b/app/components/modal_component/modal_component.html.haml
index 8c13b1dee0..1677d71b3f 100644
--- a/app/components/modal_component/modal_component.html.haml
+++ b/app/components/modal_component/modal_component.html.haml
@@ -1,4 +1,4 @@
-%div{ id: @id, "data-controller": "modal", "data-action": "keyup@document->modal#closeIfEscapeKey" }
+%div{ id: @id, "data-controller": "modal", "data-action": "keyup@document->modal#closeIfEscapeKey", "data-modal-instant-value": @instant }
   .reveal-modal-bg.fade{ "data-modal-target": "background", "data-action": "click->modal#close" }
   .reveal-modal.fade.small.modal-component{ "data-modal-target": "modal" }
     = content

From 6479990f6d2960c27984ab65d898ccba4472121f Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 11 Jan 2024 11:21:30 +1100
Subject: [PATCH 07/13] Show image update form in modal

It submits to the existing controller. I wanted to submit it with StimulusReflex, but it [doesn't support file uploads](https://docs.stimulusreflex.com/guide/working-with-forms.html#a-note-about-file-uploads). Perhaps we'll enhance this with javascript later.
---
 .../modal_component/modal_component.scss         | 10 ++++++++++
 app/controllers/spree/admin/images_controller.rb |  2 +-
 app/reflexes/products_reflex.rb                  | 15 +++++++++++++++
 .../admin/products_v3/_edit_image.html.haml      | 16 ++++++++++++++++
 app/views/admin/products_v3/_table.html.haml     |  2 +-
 app/views/admin/products_v3/index.html.haml      |  2 ++
 config/locales/en.yml                            |  3 +++
 7 files changed, 48 insertions(+), 2 deletions(-)
 create mode 100644 app/views/admin/products_v3/_edit_image.html.haml

diff --git a/app/components/modal_component/modal_component.scss b/app/components/modal_component/modal_component.scss
index 5223d23824..5e9f6ad8bd 100644
--- a/app/components/modal_component/modal_component.scss
+++ b/app/components/modal_component/modal_component.scss
@@ -6,6 +6,16 @@
   &.in {
     padding: 1.2rem;
   }
+
+  h1,
+  h2,
+  h3,
+  h4,
+  h5,
+  h6,
+  p {
+    margin-bottom: 0.5em;
+  }
 }
 
 /* prevent arrow on selected admin menu item appearing above modal */
diff --git a/app/controllers/spree/admin/images_controller.rb b/app/controllers/spree/admin/images_controller.rb
index 89e2087c0b..44b26278f8 100644
--- a/app/controllers/spree/admin/images_controller.rb
+++ b/app/controllers/spree/admin/images_controller.rb
@@ -76,7 +76,7 @@ module Spree
       end
 
       def location_after_save
-        spree.admin_product_images_url(params[:product_id], @url_filters)
+        params[:return_url] || spree.admin_product_images_url(params[:product_id], @url_filters)
       end
 
       def load_data
diff --git a/app/reflexes/products_reflex.rb b/app/reflexes/products_reflex.rb
index 711f4a577c..01f185914f 100644
--- a/app/reflexes/products_reflex.rb
+++ b/app/reflexes/products_reflex.rb
@@ -74,6 +74,15 @@ class ProductsReflex < ApplicationReflex
     fetch_and_render_products_with_flash
   end
 
+  def edit_image
+    id = current_id_from_element(element)
+    product = product_finder(id).find_product
+
+    morph "#modal-component",
+          render(partial: "admin/products_v3/edit_image",
+                 locals: { product:, image: product.image, return_url: url })
+  end
+
   private
 
   def init_filters_params
@@ -232,4 +241,10 @@ class ProductsReflex < ApplicationReflex
   def current_id_from_element(element)
     element.dataset.current_id
   end
+
+  def image_params
+    params.require(:image).permit(
+      :attachment, :viewable_id
+    )
+  end
 end
diff --git a/app/views/admin/products_v3/_edit_image.html.haml b/app/views/admin/products_v3/_edit_image.html.haml
new file mode 100644
index 0000000000..2f64a278ec
--- /dev/null
+++ b/app/views/admin/products_v3/_edit_image.html.haml
@@ -0,0 +1,16 @@
+= render ModalComponent.new id: "#modal_edit_product_image_#{image.id}", instant: true do
+  %h2= t(".title")
+
+  %p= image_tag image.variant(:product)
+
+  -# Submit to controller, because StimulusReflex doesn't support file uploads
+  = form_for [:admin, product, image], url: admin_product_image_path(product, image),
+      html: { multipart: true } do |f|
+    %input{ type: :hidden, name: :return_url, value: return_url}
+    = f.hidden_field :viewable_id, value: product.id
+
+    %p
+      -# label.button provides a handy shortcut to open the file selector on click. Unfortunately this trick isn't keyboard accessible though..
+      = f.label :attachment, t(".upload"), class: "button primary icon-upload-alt"
+      = f.file_field :attachment, style: "display: none"
+      = f.submit # todo: submit automatically
diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 68dfa43cff..4ad0d5663e 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -52,7 +52,7 @@
               .image-field
                 = image_tag product.image&.url(:mini) || Spree::Image.default_image_url(:mini), width: 40, height: 40
                 - if product.image.present?
-                  = link_to t('admin.products_page.image.edit'), edit_admin_product_image_path(product, product.image), class: "button secondary mini"
+                  = link_to t('admin.products_page.image.edit'), edit_admin_product_image_path(product, product.image), class: "button secondary mini", "data-controller": "modal", "data-reflex": "click->products#edit_image", "data-current-id": product.id
             %td.field.align-left.header
               = product_form.hidden_field :id
               = product_form.text_field :name, 'aria-label': t('admin.products_page.columns.name')
diff --git a/app/views/admin/products_v3/index.html.haml b/app/views/admin/products_v3/index.html.haml
index a7833c22df..52ca21def8 100644
--- a/app/views/admin/products_v3/index.html.haml
+++ b/app/views/admin/products_v3/index.html.haml
@@ -19,3 +19,5 @@
   #products-content
   - %w[product variant].each do |object_type|
     = render partial: 'delete_modal', locals: { object_type: }
+  #modal-component
+
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 1838426b45..b8ac7d6fb3 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -872,6 +872,9 @@ en:
         reset: Discard changes
       bulk_update:
         success: Changes saved
+      edit_image:
+        title: Edit product photo
+        upload: Upload photo
       delete_product:
         success: Successfully deleted the product
         error: Unable to delete the product

From 6ed96bae857ec820a1db0d682346a10b11d57ff5 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 11 Jan 2024 15:00:15 +1100
Subject: [PATCH 08/13] Submit image when selected

This could have been done with a tiny amount of inline JS, but I went this way in case I needed any special logic for UJS. It turns out we don't.. but it still looks a bit cleaner this way.
---
 app/views/admin/products_v3/_edit_image.html.haml | 5 ++---
 app/webpacker/controllers/form_controller.js      | 7 +++++++
 2 files changed, 9 insertions(+), 3 deletions(-)
 create mode 100644 app/webpacker/controllers/form_controller.js

diff --git a/app/views/admin/products_v3/_edit_image.html.haml b/app/views/admin/products_v3/_edit_image.html.haml
index 2f64a278ec..6edaf8d5cb 100644
--- a/app/views/admin/products_v3/_edit_image.html.haml
+++ b/app/views/admin/products_v3/_edit_image.html.haml
@@ -5,12 +5,11 @@
 
   -# Submit to controller, because StimulusReflex doesn't support file uploads
   = form_for [:admin, product, image], url: admin_product_image_path(product, image),
-      html: { multipart: true } do |f|
+      html: { multipart: true }, data: { controller: "form" } do |f|
     %input{ type: :hidden, name: :return_url, value: return_url}
     = f.hidden_field :viewable_id, value: product.id
 
     %p
       -# label.button provides a handy shortcut to open the file selector on click. Unfortunately this trick isn't keyboard accessible though..
       = f.label :attachment, t(".upload"), class: "button primary icon-upload-alt"
-      = f.file_field :attachment, style: "display: none"
-      = f.submit # todo: submit automatically
+      = f.file_field :attachment, style: "display: none", "data-action": "change->form#submit"
diff --git a/app/webpacker/controllers/form_controller.js b/app/webpacker/controllers/form_controller.js
new file mode 100644
index 0000000000..aaac9db4ec
--- /dev/null
+++ b/app/webpacker/controllers/form_controller.js
@@ -0,0 +1,7 @@
+import { Controller } from "stimulus";
+
+export default class FormController extends Controller {
+  submit() {
+    this.element.submit();
+  }
+}

From 88c840b3ee50261ec0559d48d5a8b0c31e55f92c Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 11 Jan 2024 15:20:12 +1100
Subject: [PATCH 09/13] Update styling for close button

I think we need a more general solution similar to ConfirmModalComponent's .modal-actions so that we can style all modals more consistently. But it's too late in the afternoon.
---
 app/views/admin/products_v3/_edit_image.html.haml | 3 ++-
 config/locales/en.yml                             | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/views/admin/products_v3/_edit_image.html.haml b/app/views/admin/products_v3/_edit_image.html.haml
index 6edaf8d5cb..5c940979e4 100644
--- a/app/views/admin/products_v3/_edit_image.html.haml
+++ b/app/views/admin/products_v3/_edit_image.html.haml
@@ -1,4 +1,4 @@
-= render ModalComponent.new id: "#modal_edit_product_image_#{image.id}", instant: true do
+= render ModalComponent.new id: "#modal_edit_product_image_#{image.id}", instant: true, close_button: false do
   %h2= t(".title")
 
   %p= image_tag image.variant(:product)
@@ -10,6 +10,7 @@
     = f.hidden_field :viewable_id, value: product.id
 
     %p
+      %input{ class: "secondary", type: 'button', value: t('.close'), "data-action": "click->modal#close" }
       -# label.button provides a handy shortcut to open the file selector on click. Unfortunately this trick isn't keyboard accessible though..
       = f.label :attachment, t(".upload"), class: "button primary icon-upload-alt"
       = f.file_field :attachment, style: "display: none", "data-action": "change->form#submit"
diff --git a/config/locales/en.yml b/config/locales/en.yml
index b8ac7d6fb3..bd1e28d44e 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -874,6 +874,7 @@ en:
         success: Changes saved
       edit_image:
         title: Edit product photo
+        close: Back
         upload: Upload photo
       delete_product:
         success: Successfully deleted the product

From 471dadeaec756b3bab5c87de737b15e82acaf224 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 12 Jan 2024 14:11:15 +1100
Subject: [PATCH 10/13] Accept only image files

Leaving the more specific validation in the Rails model (rather than try to duplicate it).
---
 app/views/admin/products_v3/_edit_image.html.haml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/views/admin/products_v3/_edit_image.html.haml b/app/views/admin/products_v3/_edit_image.html.haml
index 5c940979e4..7af268d64d 100644
--- a/app/views/admin/products_v3/_edit_image.html.haml
+++ b/app/views/admin/products_v3/_edit_image.html.haml
@@ -13,4 +13,4 @@
       %input{ class: "secondary", type: 'button', value: t('.close'), "data-action": "click->modal#close" }
       -# label.button provides a handy shortcut to open the file selector on click. Unfortunately this trick isn't keyboard accessible though..
       = f.label :attachment, t(".upload"), class: "button primary icon-upload-alt"
-      = f.file_field :attachment, style: "display: none", "data-action": "change->form#submit"
+      = f.file_field :attachment, accept: "image/*", style: "display: none", "data-action": "change->form#submit"

From fa9deeb948527e90f21659769e428cbf2b63c88f Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 16 Jan 2024 10:30:39 +1100
Subject: [PATCH 11/13] Remove unused method

---
 app/reflexes/products_reflex.rb | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/app/reflexes/products_reflex.rb b/app/reflexes/products_reflex.rb
index 01f185914f..ec6c28f9fd 100644
--- a/app/reflexes/products_reflex.rb
+++ b/app/reflexes/products_reflex.rb
@@ -241,10 +241,4 @@ class ProductsReflex < ApplicationReflex
   def current_id_from_element(element)
     element.dataset.current_id
   end
-
-  def image_params
-    params.require(:image).permit(
-      :attachment, :viewable_id
-    )
-  end
 end

From c67f0c52a2a4c54fc55ae276a7322f169ed6bdf1 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 16 Jan 2024 13:14:04 +1100
Subject: [PATCH 12/13] Show edit button only when hover image

This required some branching in the haml, which would be nice to avoid. But we have more work to do here so we'll refactor at the end.
---
 app/views/admin/products_v3/_table.html.haml | 11 +++++++----
 app/webpacker/css/admin/products_v3.scss     | 16 ++++++++++------
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 4ad0d5663e..c508972722 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -49,10 +49,13 @@
         %tbody.relaxed{ 'data-record-id': product_form.object.id }
           %tr
             %td.with-image
-              .image-field
-                = image_tag product.image&.url(:mini) || Spree::Image.default_image_url(:mini), width: 40, height: 40
-                - if product.image.present?
-                  = link_to t('admin.products_page.image.edit'), edit_admin_product_image_path(product, product.image), class: "button secondary mini", "data-controller": "modal", "data-reflex": "click->products#edit_image", "data-current-id": product.id
+              - if product.image.present?
+                %a.image-field{ href: edit_admin_product_image_path(product, product.image), data: { controller: "modal", reflex: "click->products#edit_image", "current-id": product.id} }
+                  = image_tag product.image&.url(:mini), width: 40, height: 40
+                  .button.secondary.mini= t('admin.products_page.image.edit')
+              - else
+                .image-field
+                  = image_tag Spree::Image.default_image_url(:mini), width: 40, height: 40
             %td.field.align-left.header
               = product_form.hidden_field :id
               = product_form.text_field :name, 'aria-label': t('admin.products_page.columns.name')
diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index c03f8ee016..6316027db0 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -88,10 +88,6 @@
           height: 100%;
         }
       }
-
-      .image-field .button {
-        display: block;
-      }
     }
 
     th,
@@ -379,7 +375,7 @@
     }
   }
 
-  .image-field {
+  a.image-field {
     position: relative;
 
     img {
@@ -388,10 +384,18 @@
     }
 
     .button {
-      display: none; // to be shown on tr:hover
+      display: none; // to be shown on hover
       position: absolute;
       bottom: 0;
       left: 0;
     }
+
+    &:hover,
+    &:focus {
+      .button.secondary {
+        display: block;
+        background-color: $color-btn-secondary-hover-bg;
+      }
+    }
   }
 }

From 627863bdf7aece7d3cf49dfde90e1a81c570bf2d Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 16 Jan 2024 13:39:02 +1100
Subject: [PATCH 13/13] Fix up button colour variables

---
 app/webpacker/css/admin/products_v3.scss           |  2 ++
 app/webpacker/css/admin_v3/components/buttons.scss | 14 +++++---------
 app/webpacker/css/admin_v3/globals/variables.scss  |  2 +-
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index 6316027db0..2ac6a85eff 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -395,6 +395,8 @@
       .button.secondary {
         display: block;
         background-color: $color-btn-secondary-hover-bg;
+        color: $color-btn-hover-bg;
+        border-color: $color-btn-hover-border;
       }
     }
   }
diff --git a/app/webpacker/css/admin_v3/components/buttons.scss b/app/webpacker/css/admin_v3/components/buttons.scss
index 37f3d9d929..0215f75952 100644
--- a/app/webpacker/css/admin_v3/components/buttons.scss
+++ b/app/webpacker/css/admin_v3/components/buttons.scss
@@ -54,20 +54,16 @@ button:not(.plain):not(.trix-button),
 
   &.secondary {
     background-color: $color-btn-secondary-bg;
-    border: 1px solid $color-btn-bg;
-    color: $color-btn-bg;
+    color: $color-link;
 
     &:hover {
-      background-color: $color-11;
-      border: 1px solid $color-btn-secondary-hover-bg;
-      color: $color-btn-secondary-hover-bg;
+      background-color: $color-btn-secondary-hover-bg;
     }
 
-    &:active,
     &:focus {
-      background-color: $color-11;
-      border: 1px solid $color-4;
-      color: $color-4;
+      background-color: $color-btn-secondary-hover-bg;
+      border-color: $color-link-focus;
+      color: $color-link-focus;
     }
   }
 
diff --git a/app/webpacker/css/admin_v3/globals/variables.scss b/app/webpacker/css/admin_v3/globals/variables.scss
index feff9ef373..29d4342278 100644
--- a/app/webpacker/css/admin_v3/globals/variables.scss
+++ b/app/webpacker/css/admin_v3/globals/variables.scss
@@ -54,7 +54,7 @@ $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;
+$color-btn-secondary-hover-bg:   $mystic !default;
 
 // Actions colors
 $color-action-edit-bg:           very-light($teal) !default;