-
-
Notifications
You must be signed in to change notification settings - Fork 719
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.
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
[BUU] Change product columns to be shown #12546
[BUU] Change product columns to be shown #12546
Conversation
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.
0e520b6
to
2f06333
Compare
2f06333
to
2a1678e
Compare
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.
2a1678e
to
2df7257
Compare
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.
0888490
to
1d3c97f
Compare
For some reason, minimum widths work now (I swear they didn't before). Hmm i would really like to shorten that stimulus controller name.
@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: |
Thanks Rachel for testing, and Maikel for responding. |
you got there before me! I provided the same feedback, can see David is on it. thanks! |
@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!
91a4fa7
to
aff50f6
Compare
looking good on my side too! tested on brave, looks shiny 🤩 |
Thanks for confirming. @RachL this is ready to test now. |
This comment was marked as outdated.
This comment was marked as outdated.
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; } }
Fixed first issue:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
|
||
this.#showHideElement(cell, element.checked); | ||
}); | ||
this.table.classList.toggle(`hide-${name}`, !element.checked); |
There was a problem hiding this comment.
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. 😉
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 😜
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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..
Thanks to Maikel's quick review, I think we can go straight on to testing again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 👍🏻
@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 |
@RachL thanks for testing. I accidentally missed configuring one of the columns, all fixed now, so I will merge. |
What? Why?
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.
Other styles not broken, and a bit more consistent with v3 style: