-
-
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
Merged
dacook
merged 25 commits into
openfoodfoundation:master
from
dacook:buu/change-columns-11055
Jun 18, 2024
Merged
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
2d0f206
Prepare spec
dacook d34e7db
Specify accepted format on client side
dacook 9c0f55a
Refactor
dacook 68da9c9
Add form to save column preferences
dacook 50469fe
Use consistent translation keys
dacook 89cedc4
Submit and render with Turbo Stream
dacook d81c3cb
Show/hide columns based on checkboxes
dacook ae66a85
Show error messages
dacook e516e7f
Tweak checkbox dropdown styles
dacook c6452ef
Isolate styles for their intended use
dacook 9ba3b4f
Fix up styles
dacook 8c75e6b
Make column selector a dropdown
dacook 0190d6f
Update dropdown styles
dacook 70fab2b
Show/hide columns using display instead of visibility
dacook d5456a8
Reset cell colspans
dacook 9ae4d34
Update widths
dacook e7774d7
Lint fix
dacook a7ef243
Enable all columns by default
dacook db27fc5
Remove dead code
dacook b25d2ed
Refactor to fix Metrics/AbcSize linter
dacook 5d0e241
Pending spec
dacook 3c9f77d
Restore image display with absolute width
mkllnk aff50f6
Name and Producer columns grow to fill and other tweaks
dacook b8f8d6d
Show/hide columns with CSS selectors instead
dacook 6fd3cad
Fix classname
dacook File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
app/views/spree/admin/shared/_stimulus_sortable_header.html.haml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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..