Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BUU] Change product columns to be shown #12546

Merged
merged 25 commits into from
Jun 18, 2024
Merged

[BUU] Change product columns to be shown #12546

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
2d0f206
Prepare spec
dacook Jun 4, 2024
d34e7db
Specify accepted format on client side
dacook Jun 4, 2024
9c0f55a
Refactor
dacook Jun 4, 2024
68da9c9
Add form to save column preferences
dacook Jun 3, 2024
50469fe
Use consistent translation keys
dacook Jun 4, 2024
89cedc4
Submit and render with Turbo Stream
dacook Jun 4, 2024
d81c3cb
Show/hide columns based on checkboxes
dacook Jun 5, 2024
ae66a85
Show error messages
dacook Jun 5, 2024
e516e7f
Tweak checkbox dropdown styles
dacook Jun 5, 2024
c6452ef
Isolate styles for their intended use
dacook Jun 11, 2024
9ba3b4f
Fix up styles
dacook Jun 11, 2024
8c75e6b
Make column selector a dropdown
dacook Jun 11, 2024
0190d6f
Update dropdown styles
dacook Jun 11, 2024
70fab2b
Show/hide columns using display instead of visibility
dacook Jun 12, 2024
d5456a8
Reset cell colspans
dacook Jun 12, 2024
9ae4d34
Update widths
dacook Jun 12, 2024
e7774d7
Lint fix
dacook Jun 13, 2024
a7ef243
Enable all columns by default
dacook Jun 13, 2024
db27fc5
Remove dead code
dacook Jun 13, 2024
b25d2ed
Refactor to fix Metrics/AbcSize linter
dacook Jun 13, 2024
5d0e241
Pending spec
dacook Jun 13, 2024
3c9f77d
Restore image display with absolute width
mkllnk Jun 13, 2024
aff50f6
Name and Producer columns grow to fill and other tweaks
dacook Jun 17, 2024
b8f8d6d
Show/hide columns with CSS selectors instead
dacook Jun 18, 2024
6fd3cad
Fix classname
dacook Jun 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions app/views/admin/products_v3/_product_row.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
%td.with-image{ id: "image-#{product.id}" }
%td.col-image.with-image{ id: "image-#{product.id}" }
= render partial: "product_image", locals: { product: }
%td.field.align-left.header.naked_inputs
%td.col-name.field.align-left.header.naked_inputs
= f.hidden_field :id
= f.text_field :name, 'aria-label': t('admin.products_page.columns.name')
= error_message_on product, :name
%td.field.naked_inputs
%td.col-sku.field.naked_inputs
= f.text_field :sku, 'aria-label': t('admin.products_page.columns.sku')
= error_message_on product, :sku
%td.field.naked_inputs{ 'data-controller': 'toggle-control', 'data-toggle-control-match-value': 'items' }
%td.col-unit_scale.field.naked_inputs{ 'data-controller': 'toggle-control', 'data-toggle-control-match-value': 'items' }
= f.hidden_field :variant_unit
= f.hidden_field :variant_unit_scale
= f.select :variant_unit_with_scale,
Expand All @@ -19,23 +19,23 @@
.field
= f.text_field :variant_unit_name, 'aria-label': t('items'), 'data-toggle-control-target': 'control', style: (product.variant_unit == "items" ? "" : "display: none")
= error_message_on product, :variant_unit_name, 'data-toggle-control-target': 'control'
%td.align-right
%td.col-unit.align-right
-# empty
%td.align-right
%td.col-price.align-right
-# empty
%td.align-right
%td.col-on_demand.align-right
-# empty
%td.naked_inputs
%td.col-producer.naked_inputs
= render(SearchableDropdownComponent.new(form: f,
name: :supplier_id,
aria_label: t('.producer_field_name'),
options: producer_options,
selected_option: product.supplier_id,
placeholder_value: t('admin.products_v3.filters.search_for_producers')))
%td.align-left
%td.col-category.align-left
-# empty
%td.align-left
%td.align-left
%td.col-tax_category.align-left
%td.col-inherits_properties.align-left
.content= product.inherits_properties ? 'YES' : 'NO' #TODO: consider using https://github.com/RST-J/human_attribute_values, else use I18n.t (also below)
%td.align-right
= render(VerticalEllipsisMenu::Component.new) do
Expand Down
42 changes: 21 additions & 21 deletions app/views/admin/products_v3/_table.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,17 @@
%table.products{ 'data-column-preferences-target': "table" }
%colgroup
-# The `min-width` property works in Chrome but not Firefox so is considered progressive enhancement.
%col{ 'data-column-preferences-name': :image, width:"56px" }= # (image size + padding)
%col{ 'data-column-preferences-name': :name, style:"min-width: 6em" }= # (grow to fill)
%col{ 'data-column-preferences-name': :sku, width:"8%", style:"min-width: 6em" }
%col{ 'data-column-preferences-name': :unit_scale, width:"8%" }
%col{ 'data-column-preferences-name': :unit, width:"8%" }
%col{ 'data-column-preferences-name': :price, width:"5%", style:"min-width: 5em" }
%col{ 'data-column-preferences-name': :on_hand, width:"10%"}
%col{ 'data-column-preferences-name': :producer, style:"min-width: 6em" }= # (grow to fill)
%col{ 'data-column-preferences-name': :category, width:"8%" }
%col{ 'data-column-preferences-name': :tax_category, width:"8%" }
%col{ 'data-column-preferences-name': :inherits_properties, width:"5%" }
%col.col-image{ width:"56px" }= # (image size + padding)
%col.col-name{ style:"min-width: 6em" }= # (grow to fill)
%col.col-sku{ width:"8%", style:"min-width: 6em" }
%col.col-unit_scale{ width:"8%" }
%col.col-unit{ width:"8%" }
%col.col-price{ width:"5%", style:"min-width: 5em" }
%col.col-on_hand{ width:"10%"}
%col.col-producer{ style:"min-width: 6em" }= # (grow to fill)
%col.col-category{ width:"8%" }
%col.col-tax_category{ width:"8%" }
%col.col-inherits_properties{ width:"5%" }
%col{ width:"5%", style:"min-width: 3em"}= # Actions
%thead
%tr
Expand All @@ -49,18 +49,18 @@
= t('.reset')
= form.submit t('.save'), class: "medium"
%tr
%th.align-left= # image
%th.col-image.align-left= # image
= render partial: 'spree/admin/shared/stimulus_sortable_header',
locals: { column: :name, sorted: params.dig(:q, :s), default: 'name asc' }
%th.align-left.with-input= t('admin.products_page.columns.sku')
%th.align-left.with-input= t('admin.products_page.columns.unit_scale')
%th.align-left.with-input= t('admin.products_page.columns.unit')
%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')
%th.align-left= t('admin.products_page.columns.inherits_properties')
%th.align-left.col-sku.with-input= t('admin.products_page.columns.sku')
%th.align-left.col-unit_scale.with-input= t('admin.products_page.columns.unit_scale')
%th.align-left.col-unit.with-input= t('admin.products_page.columns.unit')
%th.align-left.col-price.with-input= t('admin.products_page.columns.price')
%th.align-left.col-on_hand.with-input= t('admin.products_page.columns.on_hand')
%th.align-left.col-producer= t('admin.products_page.columns.producer')
%th.align-left.col-category= t('admin.products_page.columns.category')
%th.align-left.col-tax_category= t('admin.products_page.columns.tax_category')
%th.align-left.col-inherits_properties= t('admin.products_page.columns.inherits_properties')
%th.align-right= t('admin.products_page.columns.actions')
- products.each_with_index do |product, product_index|
= form.fields_for("products", product, index: product_index) do |product_form|
Expand Down
22 changes: 11 additions & 11 deletions app/views/admin/products_v3/_variant_row.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
%td
%td.col-image
-# empty
%td.field.naked_inputs
%td.col-name.field.naked_inputs
= f.hidden_field :id
= f.text_field :display_name, 'aria-label': t('admin.products_page.columns.name'), placeholder: variant.product.name
= error_message_on variant, :display_name
%td.field.naked_inputs
%td.col-sku.field.naked_inputs
= f.text_field :sku, 'aria-label': t('admin.products_page.columns.sku')
= error_message_on variant, :sku
%td
%td.col-unit_scale
-# empty
%td.field.popout{'data-controller': "popout", 'data-popout-update-display-value': "false"}
%td.col-unit.field.popout{'data-controller': "popout", 'data-popout-update-display-value': "false"}
= f.button :unit_to_display, class: "popout__button", 'aria-label': t('admin.products_page.columns.unit'), 'data-popout-target': "button" do
= variant.unit_to_display # Show the generated summary of unit values
%div.popout__container{ style: 'display: none;', 'data-controller': 'toggle-control', 'data-popout-target': "dialog" }
Expand All @@ -25,10 +25,10 @@
= f.label :display_as, t('admin.products_page.columns.display_as')
= f.text_field :display_as, placeholder: VariantUnits::OptionValueNamer.new(variant).name
= error_message_on variant, :unit_value
%td.field.naked_inputs
%td.col-price.field.naked_inputs
= f.text_field :price, 'aria-label': t('admin.products_page.columns.price'), value: number_to_currency(variant.price, unit: '')&.strip # TODO: add a spec to prove that this formatting is necessary. If so, it should be in a shared form helper for currency inputs
= error_message_on variant, :price
%td.field.popout{'data-controller': "popout"}
%td.col-on_hand.field.popout{'data-controller': "popout"}
%button.popout__button{'data-popout-target': "button", 'aria-label': t('admin.products_page.columns.on_hand')}
= variant.on_demand ? t(:on_demand) : variant.on_hand
%div.popout__container{ style: 'display: none;', 'data-controller': 'toggle-control', 'data-popout-target': "dialog" }
Expand All @@ -39,16 +39,16 @@
= f.label :on_demand do
= f.check_box :on_demand, 'data-action': 'change->toggle-control#disableIfPresent change->popout#closeIfChecked'
= t(:on_demand)
%td.align-left
%td.col-producer.align-left
-# empty producer name
%td.field.naked_inputs
%td.col-category.field.naked_inputs
= render(SearchableDropdownComponent.new(form: f,
name: :primary_taxon_id,
options: category_options,
selected_option: variant.primary_taxon_id,
aria_label: t('.category_field_name'),
placeholder_value: t('admin.products_v3.filters.search_for_categories')))
%td.field.naked_inputs
%td.col-tax_category.field.naked_inputs
= render(SearchableDropdownComponent.new(form: f,
name: :tax_category_id,
options: tax_category_options,
Expand All @@ -57,7 +57,7 @@
aria_label: t('.tax_category_field_name'),
placeholder_value: t('.search_for_tax_categories')))
= error_message_on variant, :tax_category
%td.align-left
%td.col-inherits_properties.align-left
-# empty
%td.align-right
= render(VerticalEllipsisMenu::Component.new) do
Expand Down
2 changes: 1 addition & 1 deletion app/views/spree/admin/shared/_stimulus_sortable_header.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
%th
%th{ class: "col-#{column}" }
%a{ "data-controller": "search", "data-action": "click->search#changeSorting", "data-column": "#{column}", "data-current": (sorted || default).to_s }
= t("spree.admin.shared.sortable_header.#{column.to_s}")

Expand Down
29 changes: 3 additions & 26 deletions app/webpacker/controllers/column_preferences_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export default class ColumnPreferencesController extends Controller {
cell.dataset.defaultColSpan ||= cell.colSpan;
});

this.checkboxes = this.element.querySelectorAll("input[type=checkbox]");
this.checkboxes = Array.from(this.element.querySelectorAll("input[type=checkbox]"));
for (const element of this.checkboxes) {
// On initial load
this.#showHideColumn(element);
Expand All @@ -26,40 +26,17 @@ export default class ColumnPreferencesController extends Controller {
#showHideColumn(e) {
const element = e.target || e;
const name = element.dataset.columnName;
const selector = `col[data-column-preferences-name="${name}"]`;
const column = this.table.querySelector(selector);
const index = this.#getIndex(column);

if (column == null) {
console.error(`ColumnPreferencesController: could not find ${selector}`);
return;
}

// Hide column definition
this.#showHideElement(column, element.checked);

// Hide each cell in column (ignore rows with colspan)
const rows = this.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);
});
this.table.classList.toggle(`hide-${name}`, !element.checked);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the next step would be to have the column selector be labels for hidden checkboxes in the columns and couple the visibility of the columns with the value of the hidden input in CSS. Then you won't need JS at all. 😉

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤯

That's so crazy it might just work

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why, but I tried it, and it worked with pretty minimal changes!
dacook@a46eded

But pretty minimal benefit too, so I won't pursue it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't know the has selector. That's clever. But as long as we need the column counter, we can't get rid of the JS. Hm, I would probably scrap the counter but I'm not the designer. 😜

Copy link
Member Author

@dacook dacook Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, unfortunately the counter is needed in two places, where i've employed colspan to make rows span. One of them I think I could probably work around, but the other requires to span all visible rows. Unfortunately there isn't a builtin solution for that. (you can use an arbitrary high number like colspan="999", which automatically spans all columns, but this doesn't take into account hidden columns).
Hmm, maybe it could be hacked from within the first column...

Edit: nope, it cant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw one example that used visibility: collapse which can hide entire columns. I haven't tested the effect on colspan though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did use visibility: collapse initially on this PR, and it worked well to hide the columns. I think it handled the colspan too, which was a big bonus. But I couldn't get the column widths to behave properly (70fab2b)

It might be worth trying it again after the latest tweaks, but I'm not keen to add more work unless it's needed..


// Reset cell colspans
const hiddenColCount = this.cols.filter((col)=> col.style.display == 'none').length;
const hiddenColCount = this.checkboxes.filter((checkbox)=> !checkbox.checked).length;
for(const cell of this.colSpanCells) {
const span = parseInt(cell.dataset.defaultColSpan, 10) - hiddenColCount;
cell.colSpan = span;
};
}

#getIndex(column) {
return Array.from(column.parentNode.children).indexOf(column);
}

#showHideElement(element, show) {
element.style.display = show ? "" : "none";
}
Expand Down
19 changes: 16 additions & 3 deletions app/webpacker/css/admin/products_v3.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@

// "Naked" inputs. Row hover helps reveal them.
.naked_inputs {
input:not([type="checkbox"]), .ts-control {
input:not([type="checkbox"]),
.ts-control {
background-color: $color-tbl-cell-bg;
}
}
Expand Down Expand Up @@ -179,6 +180,18 @@
.ts-control {
z-index: 0; // Avoid hovering over thead
}

// Hide columns
$columns:
"image", "name", "sku", "unit_scale", "unit", "price", "on_hand", "producer", "category",
"tax_category", "inherits_properties";
@each $col in $columns {
&.hide-#{$col} {
.col-#{$col} {
display: none;
}
}
}
}

#no-products {
Expand Down Expand Up @@ -378,7 +391,7 @@
border-radius: $border-radius;
box-shadow: 0px 0px 8px 0px rgba($near-black, 0.25);

.field{
.field {
margin-bottom: 0.75em;

&:last-child {
Expand Down Expand Up @@ -432,7 +445,7 @@
opacity: 0;
}
}

.slide-out {
animation: slideOutLeft 0.5s forwards;
}
Expand Down
Loading