From 70fab2bcc133200817a9fccac41eaf3e836f9bf7 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 12 Jun 2024 15:55:13 +1000
Subject: [PATCH] Show/hide columns using display instead of visibility

Visibility was way simpler, but the table doesn't recalculate column widths until you use display:none;

This is now using the same method as the old products screen.
But we still need to update colspans..
---
 .../column_preferences_controller.js          | 25 ++++++++++++++++---
 .../system/admin/products_v3/products_spec.rb |  4 +--
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/app/webpacker/controllers/column_preferences_controller.js b/app/webpacker/controllers/column_preferences_controller.js
index 31ec0bb65c..9a81faefb6 100644
--- a/app/webpacker/controllers/column_preferences_controller.js
+++ b/app/webpacker/controllers/column_preferences_controller.js
@@ -1,11 +1,10 @@
 import { Controller } from "stimulus";
 
-
 // Manage column visibility according to checkbox selection
 //
 export default class ColumnPreferencesController extends Controller {
   connect() {
-    this.checkboxes = this.element.querySelectorAll('input[type=checkbox]');
+    this.checkboxes = this.element.querySelectorAll("input[type=checkbox]");
 
     for (const element of this.checkboxes) {
       // On initial load
@@ -22,12 +21,32 @@ export default class ColumnPreferencesController extends Controller {
     const name = element.dataset.columnName;
     const selector = `col[data-column-preferences-name="${name}"]`;
     const column = document.querySelector(selector);
+    const index = this.#getIndex(column);
 
     if (column == null) {
       console.error(`ColumnPreferencesController: could not find ${selector}`);
       return;
     }
 
-    column.style.visibility = (element.checked ? '' : 'collapse');
+    // Hide column definition
+    this.#showHideElement(column, element.checked);
+
+    // Hide each cell in column (ignore rows with colspan)
+    const rows = column.closest("table").querySelectorAll("tr:not(:has(td[colspan]))");
+    rows.forEach((row) => {
+      // Ignore cell if spanning multiple columns
+      const cell = row.children[index];
+      if (cell == undefined) return;
+
+      this.#showHideElement(cell, element.checked);
+    });
+  }
+
+  #getIndex(column) {
+    return Array.from(column.parentNode.children).indexOf(column);
+  }
+
+  #showHideElement(element, show) {
+    element.style.display = show ? "" : "none";
   }
 }
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 5d76e3e522..620e1901a2 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -49,7 +49,7 @@ RSpec.describe 'As an enterprise user, I can manage my products', feature: :admi
       within ofn_drop_down("Columns") do
         uncheck "Name"
       end
-      # expect(page).not_to have_selector "th", text: "Name" # column is not visible, but capybara doesn't understand yet.
+      expect(page).not_to have_selector "th", text: "Name"
       expect_other_columns_visible
 
       # Preference saved
@@ -62,7 +62,7 @@ RSpec.describe 'As an enterprise user, I can manage my products', feature: :admi
       within ofn_drop_down("Columns") do
         expect(page).to have_unchecked_field "Name"
       end
-      # expect(page).not_to have_selector "th", text: "Name"
+      expect(page).not_to have_selector "th", text: "Name"
       expect_other_columns_visible
     end