Merge pull request #12107 from dacook/buu/fixes

[BUU] Style and behaviour updates

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 {

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%" }
@@ -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')
@@ -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

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,17 +33,33 @@ 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;
+      // Notify of change
+      this.first_input.dispatchEvent(new Event("input"));
     }
   }
 
   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.innerHTML ||= " "; // (with default space to help with styling)
       this.buttonTarget.classList.toggle("changed", this.#isChanged());
 
       this.dialogTarget.style.display = "none";

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;
     }
 
@@ -354,8 +355,8 @@
 
     &__popout {
       position: absolute;
-      top: -1em;
-      left: -1em;
+      top: -0.6em;
+      left: -0.2em;
       z-index: 1; // Cover below row when hover
       width: 9em;
 

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

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;

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
@@ -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);
@@ -89,6 +103,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", () => {

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