From ae1fabae38f5bef1423c50b45cdf4be88497796e Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 25 Jan 2024 15:02:13 +1100
Subject: [PATCH 1/8] Tweak padding according to design

---
 app/components/vertical_ellipsis_menu/component.scss | 2 +-
 app/webpacker/css/admin_v3/globals/variables.scss    | 2 +-
 app/webpacker/css/admin_v3/shared/forms.scss         | 4 +++-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/app/components/vertical_ellipsis_menu/component.scss b/app/components/vertical_ellipsis_menu/component.scss
index 9e8bc9d878..dd698ee331 100644
--- a/app/components/vertical_ellipsis_menu/component.scss
+++ b/app/components/vertical_ellipsis_menu/component.scss
@@ -7,7 +7,7 @@
     text-align: center;
     border-radius: 3px;
     background-color: white;
-    padding: 10px 14px;
+    padding: 9px 14px;
   }
 
   .vertical-ellipsis-menu-content {
diff --git a/app/webpacker/css/admin_v3/globals/variables.scss b/app/webpacker/css/admin_v3/globals/variables.scss
index 29d4342278..bbd8fde650 100644
--- a/app/webpacker/css/admin_v3/globals/variables.scss
+++ b/app/webpacker/css/admin_v3/globals/variables.scss
@@ -40,7 +40,7 @@ $color-tbl-thead-txt:            $color-headers !default;
 $color-tbl-thead-bg:             $light-grey !default;
 $color-tbl-border:               $pale-blue !default;
 $padding-tbl-cell:               12px;
-$padding-tbl-cell-condensed:     10px 12px;
+$padding-tbl-cell-condensed:     4px 12px;
 $padding-tbl-cell-relaxed:       12px 12px;
 
 // Button colors
diff --git a/app/webpacker/css/admin_v3/shared/forms.scss b/app/webpacker/css/admin_v3/shared/forms.scss
index 4a4980f0ec..b6dfa00240 100644
--- a/app/webpacker/css/admin_v3/shared/forms.scss
+++ b/app/webpacker/css/admin_v3/shared/forms.scss
@@ -16,10 +16,12 @@ input[type="number"],
 textarea,
 fieldset {
   @include border-radius($border-radius);
-  padding: $vpadding-txt $hpadding-txt;
+  padding: ($vpadding-txt - 1px) ($hpadding-txt - 1px); // Minus 1px for border
   border: 1px solid $lighter-grey;
   color: $color-txt-text;
   background-color: $lighter-grey;
+  font-size: 14px;
+  line-height: 22px;
 
   &:focus {
     outline: none;

From 3d941dcc1f9f49d35b55cfc0235708b9f0522adf Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 25 Jan 2024 15:27:06 +1100
Subject: [PATCH 2/8] Adjust popout position

Now it's lined up perfectly with the number input.
---
 app/webpacker/css/admin/products_v3.scss | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index 2ac6a85eff..11df0a7dd8 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -354,8 +354,8 @@
 
     &__popout {
       position: absolute;
-      top: -1em;
-      left: -1em;
+      top: -0.6em;
+      left: -0.2em;
       z-index: 1; // Cover below row when hover
       width: 9em;
 

From da82b12ca75123feb06416d9d1decad1ffaeeac5 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 31 Jan 2024 17:13:30 +1100
Subject: [PATCH 3/8] Prevent negative values for stock on hand

Using browser validation. I didn't use model validation because the on_hand pseudo-attribute doesn't support it.

But.. it turned out to  not be so simple. Browser validation can't work if the field is hidden, and breaks the javascript. So now I made the javascript smarter, and the end result is more helpful I think.
---
 app/views/admin/products_v3/_table.html.haml        |  2 +-
 app/webpacker/controllers/popout_controller.js      |  6 ++++++
 spec/javascripts/stimulus/popout_controller_test.js | 10 +++++++++-
 spec/system/admin/products_v3/products_spec.rb      | 10 ++++++++--
 4 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index faf517bd3e..df1108a4df 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -106,7 +106,7 @@
                     = variant.on_demand ? t(:on_demand) : variant.on_hand
                   %div.on-hand__popout{ style: 'display: none;', 'data-controller': 'toggle-control', 'data-popout-target': "dialog" }
                     .field
-                      = variant_form.number_field :on_hand, 'aria-label': t('admin.products_page.columns.on_hand'), 'data-toggle-control-target': 'control', disabled: variant_form.object.on_demand
+                      = variant_form.number_field :on_hand, min: 0, 'aria-label': t('admin.products_page.columns.on_hand'), 'data-toggle-control-target': 'control', disabled: variant_form.object.on_demand
                       = error_message_on variant, :on_hand
                     .field.checkbox
                       = variant_form.label :on_demand do
diff --git a/app/webpacker/controllers/popout_controller.js b/app/webpacker/controllers/popout_controller.js
index 0e430ce1a9..49530e7e26 100644
--- a/app/webpacker/controllers/popout_controller.js
+++ b/app/webpacker/controllers/popout_controller.js
@@ -42,6 +42,12 @@ export default class PopoutController extends Controller {
   close() {
     // Close if not already closed
     if (this.dialogTarget.style.display != "none") {
+      // Check every element for browser-side validation, before the fields get hidden.
+      if (!this.#enabledDisplayElements().every((element) => element.reportValidity())) {
+        // If any fail, don't close
+        return;
+      }
+
       // Update button to represent any changes
       this.buttonTarget.innerText = this.#displayValue();
       this.buttonTarget.classList.toggle("changed", this.#isChanged());
diff --git a/spec/javascripts/stimulus/popout_controller_test.js b/spec/javascripts/stimulus/popout_controller_test.js
index 60b6f8ce89..c90e6a90ab 100644
--- a/spec/javascripts/stimulus/popout_controller_test.js
+++ b/spec/javascripts/stimulus/popout_controller_test.js
@@ -16,7 +16,7 @@ describe("PopoutController", () => {
       <div data-controller="popout">
         <button id="button" data-popout-target="button">On demand</button>
         <div id="dialog" data-popout-target="dialog" style="display: none;">
-          <input id="input1" value="value1">
+          <input id="input1" value="value1" required>
           <label>
             <input id="input2" type="checkbox" value="value2" data-action="change->popout#closeIfChecked">
             label2
@@ -89,6 +89,14 @@ describe("PopoutController", () => {
       expect(input2.checked).toBe(false);
       expectToBeShown(dialog);
     });
+
+    it("doesn't close the dialog when a field is invalid", () => {
+      input1.value = "" // field is required
+
+      input4.click();
+      expectToBeShown(dialog);
+      // Browser will show a validation message
+    });
   });
 
   describe("Cleaning up", () => {
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 642265abea..2e506ab7f0 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -181,10 +181,16 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do
         fill_in "Name", with: "Large box"
         fill_in "SKU", with: "POM-01"
         fill_in "Price", with: "10.25"
-        click_on "On Hand" # activate stock popout
-        fill_in "On Hand", with: "6"
+
+        click_on "On Hand" # activate popout
       end
 
+      # Stock popout
+      fill_in "On Hand", with: "-1"
+      click_button "Save changes" # attempt to save or close the popout
+      expect(page).to have_field "On Hand", with: "-1" # popout is still open
+      fill_in "On Hand", with: "6"
+
       expect {
         click_button "Save changes"
         product_a.reload

From 133a9e6c7f5c23a4959f8aead8f65c265ddfae54 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 1 Feb 2024 11:14:11 +1100
Subject: [PATCH 4/8] Show popout when press printable character

---
 app/webpacker/controllers/popout_controller.js   | 13 ++++++++++---
 .../stimulus/popout_controller_test.js           | 16 +++++++++++++++-
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/app/webpacker/controllers/popout_controller.js b/app/webpacker/controllers/popout_controller.js
index 49530e7e26..0545e2d3c9 100644
--- a/app/webpacker/controllers/popout_controller.js
+++ b/app/webpacker/controllers/popout_controller.js
@@ -10,7 +10,7 @@ export default class PopoutController extends Controller {
 
     // Show when click or down-arrow on button
     this.buttonTarget.addEventListener("click", this.show.bind(this));
-    this.buttonTarget.addEventListener("keydown", this.showIfDownArrow.bind(this));
+    this.buttonTarget.addEventListener("keydown", this.applyKeyAction.bind(this));
 
     // Close when click or tab outside of dialog. Run async (don't block primary event handlers).
     this.closeIfOutsideBound = this.closeIfOutside.bind(this); // Store reference for removing listeners later.
@@ -33,9 +33,16 @@ export default class PopoutController extends Controller {
     e.preventDefault();
   }
 
-  showIfDownArrow(e) {
-    if (e.keyCode == 40) {
+  // Apply an appropriate action, behaving similar to a dropdown
+  // Shows the popout and applies the value where appropriate
+  applyKeyAction(e) {
+    if ([38, 40].includes(e.keyCode)) {
+      // Show if Up or Down arrow
       this.show(e);
+    } else if (e.key.match(/^[\d\w]$/)) {
+      // Show, and apply value if it's a digit or word character
+      this.show(e);
+      this.first_input.value = e.key;
     }
   }
 
diff --git a/spec/javascripts/stimulus/popout_controller_test.js b/spec/javascripts/stimulus/popout_controller_test.js
index c90e6a90ab..82da807bfb 100644
--- a/spec/javascripts/stimulus/popout_controller_test.js
+++ b/spec/javascripts/stimulus/popout_controller_test.js
@@ -42,7 +42,21 @@ describe("PopoutController", () => {
       expectToBeShown(dialog);
     });
 
-    it("doesn't show the dialog on other key press (tab)", () => {
+    it("shows and updates on number press", () => {
+      button.dispatchEvent(new KeyboardEvent("keydown", { key: "1" }));
+
+      expectToBeShown(dialog);
+      expect(input1.value).toBe("1");
+    });
+
+    it("shows and updates on character press", () => {
+      button.dispatchEvent(new KeyboardEvent("keydown", { key: "a" }));
+
+      expectToBeShown(dialog);
+      expect(input1.value).toBe("a");
+    });
+
+    it("doesn't show the dialog on control key press (tab)", () => {
       button.dispatchEvent(new KeyboardEvent("keydown", { keyCode: 9 }));
 
       expectToBeClosed(dialog);

From 079925bc9a931e88f98be87dfcd205d94e99515c Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 1 Feb 2024 11:31:14 +1100
Subject: [PATCH 5/8] Fix for styling with a blank value

It's not usually valid, but can still be entered. I wasn't able to fix the positioning of the :after psuedo element without having a child text node. Maybe it's possible to add an empty child text node, but I didn't think it worth getting down to that level..
---
 app/webpacker/controllers/popout_controller.js | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/app/webpacker/controllers/popout_controller.js b/app/webpacker/controllers/popout_controller.js
index 0545e2d3c9..287ed98771 100644
--- a/app/webpacker/controllers/popout_controller.js
+++ b/app/webpacker/controllers/popout_controller.js
@@ -43,6 +43,8 @@ export default class PopoutController extends Controller {
       // Show, and apply value if it's a digit or word character
       this.show(e);
       this.first_input.value = e.key;
+      // Notify of change
+      this.first_input.dispatchEvent(new Event("input"));
     }
   }
 
@@ -57,6 +59,7 @@ export default class PopoutController extends Controller {
 
       // Update button to represent any changes
       this.buttonTarget.innerText = this.#displayValue();
+      this.buttonTarget.innerHTML ||= "&nbsp;"; // (with default space to help with styling)
       this.buttonTarget.classList.toggle("changed", this.#isChanged());
 
       this.dialogTarget.style.display = "none";

From 262823bdddc23e420074478fe580dd13f8e17844 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 1 Feb 2024 11:56:40 +1100
Subject: [PATCH 6/8] Ensure On Demand field doesn't overflow following cell

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

diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index 11df0a7dd8..9cc1d1845f 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -112,6 +112,7 @@
     }
 
     td {
+      position: relative; // Ensure that overflowing content is covered by the following cell. We don't use overflow: hidden because that messes with the popover.
       background-color: $color-tbl-cell-bg;
     }
 

From 36722bfa2025f8b475018cf0ee69b7ca112e1a31 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 1 Feb 2024 12:00:40 +1100
Subject: [PATCH 7/8] Add more room for the On Hand column

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

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index df1108a4df..cde99d35ee 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -11,7 +11,7 @@
     %col{ width:"5%", style: "max-width:5em" }
     %col{ width:"8%" }
     %col{ width:"5%", style: "max-width:5em"}
-    %col{ width:"5%", style: "max-width:5em"}
+    %col{ width:"8%", style: "max-width:5em"}
     %col{ width:"10%" }= # producer
     %col{ width:"10%" }
     %col{ width:"5%" }

From d0724bab8558a931eb2d6ad7ee383c5d71922ac7 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 1 Feb 2024 12:09:49 +1100
Subject: [PATCH 8/8] Fix column header alignmnt

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

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index cde99d35ee..73b89847d4 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -35,10 +35,10 @@
       %tr
         %th.align-left= # image
         %th.align-left.with-input= t('admin.products_page.columns.name')
-        %th.align-right= t('admin.products_page.columns.sku')
+        %th.align-left.with-input= t('admin.products_page.columns.sku')
         %th.align-right= t('admin.products_page.columns.unit')
-        %th.align-right= t('admin.products_page.columns.price')
-        %th.align-right= t('admin.products_page.columns.on_hand')
+        %th.align-left.with-input= t('admin.products_page.columns.price')
+        %th.align-left.with-input= t('admin.products_page.columns.on_hand')
         %th.align-left= t('admin.products_page.columns.producer')
         %th.align-left= t('admin.products_page.columns.category')
         %th.align-left= t('admin.products_page.columns.tax_category')