[BUU] Change product columns to be shown #12546

dacook · 2024-06-04T05:35:35Z

What? Why?

Closes [BUU] Change the columns to be shown in my catalogue #11055

Add new columns dropdown selector, built with Stimulus instead of Angular (while upgrading and re-using existing styles and rails controller)
Tweak column sizing

What should we test?

Changing columns on the bulk products screen:
See requirements on attached issue.
All features from old screen should continue to work.

open/close dropdown
selecting columns will show/hide immediately
on page reload, revert to saved preferences
after saving preferences, they are retained after page reload/pagination/sort etc.

Other styles not broken, and a bit more consistent with v3 style:

bulk orders "action" dropdown
edit orders "actions" dropdown
old column column selectors (eg on bulk order management page)
reports "fields to show" dropdown
inventory "view" dropdown

I don't know why, but even though the client sends http accept header for json, rails is treating it as html. This was being overridden in the route, but I want to support multiple formats next. So, we explicitly choose the format by adding it to the request path.

Ensures that the column table names match the names in the selector. I thought 'there must be a way to set the translation scope once'. With Rails, there's usually a way. Thankfully this one was quite simple. Or is it too much magic.. 🧙 https://coderwall.com/p/dvme9q/set-scope-of-i18n-translations-in-rails-with-a-block

The cols could have been a lot cleaner with simple classnames, but I preferred to mark up in a way that reveals the purpose (otherwise they could be used for styling). It doesn't seem to be any faster comparing querySelector('[data]') vs class, or iterating through the dom nodes.

There shouldn't normally be errors, but I got one due to bad data during development, and this helped sort it out.

Use the new design for checkboxes and fix alignment. Removes redesigned-input, which is a small regression on the old design, but I think it's acceptable bcause we're going to shut it down soon.

This also improves the styling of the orders action dropdowns (on index and edit pages). It adds the new chevron icon, but needed some fiddling to make it look right.

With some styling tweaks.

The v2 dropdown is used in various places, and now looks more in line with the new design.

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..

This might be a little simpler if we move the 'new variant' button to col 0, and assume colspan cells always span the whole table.

For some reason, minimum widths work now (I swear they didn't before). Hmm i would really like to shorten that stimulus controller name.

Sorry didn't have time to go back and rebase

RachL · 2024-06-14T14:52:07Z

@mariocarabotta I'm leaving this staged on AU: can you have a look and let me know if it fits what you had in mind? Especially I'm wondering about column position when there are only a few, currently the image is taking a huge space:

With the old screen we have something like this:

dacook · 2024-06-16T23:42:41Z

Thanks Rachel for testing, and Maikel for responding.
As we can see, my latest changes which rely on min-width were required to avoid the problem Rachel has pictured. I think this is worth fixing so I will try to progress it today.

mariocarabotta · 2024-06-17T00:04:50Z

@mariocarabotta I'm leaving this staged on AU: can you have a look and let me know if it fits what you had in mind? Especially I'm wondering about column position when there are only a few, currently the image is taking a huge space:

With the old screen we have something like this:

you got there before me! I provided the same feedback, can see David is on it. thanks!

dacook · 2024-06-17T02:25:06Z

@mariocarabotta , this is ready to review again. I've re-tweaked columns and I think it's working well.

I compared with the old screen too, and I think it's a big improvement (product name and SKU were very small before).

When columns are hidden, the Name and Producer columns will grow to fill extra space. If both of those are hidden, the first column on the left will fill the space (eg image), but I think that's unlikely and acceptable.

Here's some previews I took with various

Small (1024px)

All columns (Firefox, Chrome)

You can see that in Firefox, the Price column gets cropped, but the min-width ensures more of the price shows on Chrome. (hmm I forget to check higher values like 10.00)

Some columns (Firefox, Chrome)

Large (1440px)

All columns (Firefox, Chrome)

Some columns (Firefox, Chrome)

If neither are visible, the first column on the left (eg image) will grow. But that's not a likely scenario. Min-widths help manage sizes on smaller screens in Chrome. The title for Inherits Properties gets cut off, but I think it's better than cutting off content. Oh look, it fixed a spec too!

mariocarabotta · 2024-06-17T23:56:19Z

looking good on my side too! tested on brave, looks shiny 🤩

dacook · 2024-06-18T00:02:15Z

Thanks for confirming. @RachL this is ready to test now.

mariocarabotta · 2024-06-18T03:37:01Z

sorry I've just found out that this introduces

UI bug when adding a variant inline
when opening dropdown and opening another one, the columns dropdown does not close (see shots below)

Well, that made the JS way simpler. Adds a lot of classes though. Maybe we could do it based on column index instead, but this will do for now. table.hide-col0 { td:nth-child(0) { display: none; } }

dacook · 2024-06-18T05:51:44Z

Fixed first issue:

UI bug when adding a variant inline

The second I wasn't able to fix quickly. I think we might be able to customise TomSelect to emit a standard 'click' event which would be a good generic solution.

mkllnk

Great!

mkllnk · 2024-06-18T06:47:47Z

app/webpacker/controllers/column_preferences_controller.js

-
-      this.#showHideElement(cell, element.checked);
-    });
+    this.table.classList.toggle(`hide-${name}`, !element.checked);


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. 😉

🤯

That's so crazy it might just work

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.

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. 😜

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.

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

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..

dacook · 2024-06-18T07:19:20Z

Thanks to Maikel's quick review, I think we can go straight on to testing again.

chahmedejaz

Looks great 👍🏻

RachL · 2024-06-18T12:30:41Z

@dacook the bug Mario has seen appears now when selecting 6 columns or less in desktop mode:

If you want we can merge already and fix in a new issue? In that case I will let you merge

dacook · 2024-06-18T23:25:23Z

@RachL thanks for testing. I accidentally missed configuring one of the columns, all fixed now, so I will merge.

sigmundpetersen assigned dacook Jun 4, 2024

dacook added 3 commits June 5, 2024 09:45

Prepare spec

2d0f206

Refactor

9c0f55a

dacook force-pushed the buu/change-columns-11055 branch 2 times, most recently from 0e520b6 to 2f06333 Compare June 11, 2024 05:59

dacook changed the title ~~[BUU] Change columns~~ [BUU] Change product columns to be shown Jun 11, 2024

dacook added the pr-staged-au staging.openfoodnetwork.org.au label Jun 12, 2024

dacook force-pushed the buu/change-columns-11055 branch from 2f06333 to 2a1678e Compare June 12, 2024 03:34

dacook added 8 commits June 12, 2024 14:49

Add form to save column preferences

68da9c9

Submit and render with Turbo Stream

89cedc4

Show error messages

ae66a85

There shouldn't normally be errors, but I got one due to bad data during development, and this helped sort it out.

Tweak checkbox dropdown styles

e516e7f

Use the new design for checkboxes and fix alignment. Removes redesigned-input, which is a small regression on the old design, but I think it's acceptable bcause we're going to shut it down soon.

Isolate styles for their intended use

c6452ef

This also improves the styling of the orders action dropdowns (on index and edit pages). It adds the new chevron icon, but needed some fiddling to make it look right.

Fix up styles

9ba3b4f

dacook force-pushed the buu/change-columns-11055 branch from 2a1678e to 2df7257 Compare June 12, 2024 06:58

dacook added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Jun 12, 2024

dacook added 4 commits June 13, 2024 10:07

Make column selector a dropdown

8c75e6b

With some styling tweaks.

Update dropdown styles

0190d6f

The v2 dropdown is used in various places, and now looks more in line with the new design.

Show/hide columns using display instead of visibility

70fab2b

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..

Reset cell colspans

d5456a8

This might be a little simpler if we move the 'new variant' button to col 0, and assume colspan cells always span the whole table.

dacook force-pushed the buu/change-columns-11055 branch from 0888490 to 1d3c97f Compare June 13, 2024 00:16

dacook added the pr-staged-au staging.openfoodnetwork.org.au label Jun 13, 2024

dacook added 3 commits June 13, 2024 10:36

Update widths

9ae4d34

For some reason, minimum widths work now (I swear they didn't before). Hmm i would really like to shorten that stimulus controller name.

Lint fix

e7774d7

Sorry didn't have time to go back and rebase

Enable all columns by default

a7ef243

mariocarabotta added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Jun 16, 2024

dacook force-pushed the buu/change-columns-11055 branch from 91a4fa7 to aff50f6 Compare June 17, 2024 02:27

dacook added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Jun 17, 2024

This comment was marked as outdated.

Sign in to view

Show/hide columns with CSS selectors instead

b8f8d6d

Well, that made the JS way simpler. Adds a lot of classes though. Maybe we could do it based on column index instead, but this will do for now. table.hide-col0 { td:nth-child(0) { display: none; } }

dacook requested review from mkllnk and chahmedejaz June 18, 2024 06:18

dacook removed the pr-staged-au staging.openfoodnetwork.org.au label Jun 18, 2024

mkllnk approved these changes Jun 18, 2024

View reviewed changes

chahmedejaz approved these changes Jun 18, 2024

View reviewed changes

RachL added the pr-staged-au staging.openfoodnetwork.org.au label Jun 18, 2024

RachL added feedback-needed and removed pr-staged-au staging.openfoodnetwork.org.au labels Jun 18, 2024

Fix classname

6fd3cad

dacook merged commit 2676891 into openfoodfoundation:master Jun 18, 2024
50 checks passed

dacook removed the feedback-needed label Jun 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUU] Change product columns to be shown #12546

[BUU] Change product columns to be shown #12546

dacook commented Jun 4, 2024 •

edited

Loading

RachL commented Jun 14, 2024

dacook commented Jun 16, 2024

mariocarabotta commented Jun 17, 2024

dacook commented Jun 17, 2024 •

edited

Loading

mariocarabotta commented Jun 17, 2024

dacook commented Jun 18, 2024

mariocarabotta commented Jun 18, 2024 •

edited

Loading

This comment was marked as outdated.

dacook commented Jun 18, 2024

mkllnk left a comment

mkllnk Jun 18, 2024

dacook Jun 18, 2024

dacook Jun 18, 2024

mkllnk Jun 19, 2024

dacook Jun 19, 2024 •

edited

Loading

mkllnk Jun 19, 2024

dacook Jun 19, 2024

dacook commented Jun 18, 2024

chahmedejaz left a comment

RachL commented Jun 18, 2024 •

edited

Loading

dacook commented Jun 18, 2024

[BUU] Change product columns to be shown #12546

[BUU] Change product columns to be shown #12546

Conversation

dacook commented Jun 4, 2024 • edited Loading

What? Why?

What should we test?

RachL commented Jun 14, 2024

dacook commented Jun 16, 2024

mariocarabotta commented Jun 17, 2024

dacook commented Jun 17, 2024 • edited Loading

Small (1024px)

All columns (Firefox, Chrome)

Some columns (Firefox, Chrome)

Large (1440px)

All columns (Firefox, Chrome)

Some columns (Firefox, Chrome)

mariocarabotta commented Jun 17, 2024

dacook commented Jun 18, 2024

mariocarabotta commented Jun 18, 2024 • edited Loading

This comment was marked as outdated.

dacook commented Jun 18, 2024

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk Jun 18, 2024

Choose a reason for hiding this comment

dacook Jun 18, 2024

Choose a reason for hiding this comment

dacook Jun 18, 2024

Choose a reason for hiding this comment

mkllnk Jun 19, 2024

Choose a reason for hiding this comment

dacook Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

mkllnk Jun 19, 2024

Choose a reason for hiding this comment

dacook Jun 19, 2024

Choose a reason for hiding this comment

dacook commented Jun 18, 2024

chahmedejaz left a comment

Choose a reason for hiding this comment

RachL commented Jun 18, 2024 • edited Loading

dacook commented Jun 18, 2024

dacook commented Jun 4, 2024 •

edited

Loading

dacook commented Jun 17, 2024 •

edited

Loading

mariocarabotta commented Jun 18, 2024 •

edited

Loading

dacook Jun 19, 2024 •

edited

Loading

RachL commented Jun 18, 2024 •

edited

Loading