-
-
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
[BUU2] Hide producer column when there's only one producer in the admin account #12854
[BUU2] Hide producer column when there's only one producer in the admin account #12854
Conversation
def prepare_new_variant(product, producer_options) | ||
# e.g producer_options = [['producer name', id]] | ||
product.variants.build do |new_variant| | ||
new_variant.supplier_id = producer_options.first.second if producer_options.one? |
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.
If only one producer exists, the producer column will be hidden according to the issue. In that case, the producer should be auto-assigned so that UI doesn't show an error on saving the variant.
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.
It would be nicer to have producer_options
as an array of hashes: [ { name: 'producer name', id: id } ]
, anyway its fine to keep it like this.
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.
It's a good start but I think we can improve/clarify a few things
app/models/column_preference.rb
Outdated
@@ -15,10 +15,11 @@ class ColumnPreference < ApplicationRecord | |||
validates :column_name, presence: true, inclusion: { in: proc { |p| | |||
valid_columns_for(p.action_name) | |||
} } | |||
scope :products, -> { where(action_name: 'products_v3_index') } |
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 think we should rename this to bulk_edit_product
so it's clear we are talking about a page and not a product.
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.
It's fixed here: 5be53a4
def prepare_new_variant(product, producer_options) | ||
# e.g producer_options = [['producer name', id]] | ||
product.variants.build do |new_variant| | ||
new_variant.supplier_id = producer_options.first.second if producer_options.one? |
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.
It would be nicer to have producer_options
as an array of hashes: [ { name: 'producer name', id: id } ]
, anyway its fine to keep it like this.
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 has been on the list of things to do for a long time! I like the solution using the column defaults: it means the user can still choose to show it if they want to.
But it looks like there's some leftover code to remove?
# if user hasn't saved any preferences on products page and there's only one producer; | ||
# we need to hide producer column | ||
def hide_producer_column?(producer_options) | ||
spree_current_user.column_preferences.bulk_edit_product.empty? && producer_options.one? |
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.
It seems unnecessary to create a scope for this single use. I think using the where
command directly would have been fine here.
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 think scopes are a more cleaner way to query, don't you think? 😅
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 guess it's a matter of personal preference. In general, yes it's better to use a scope which ensures the logic about the model, is in the model 👍
@@ -13,7 +13,7 @@ | |||
= hidden_field_tag :producer_id, @producer_id | |||
= hidden_field_tag :category_id, @category_id | |||
|
|||
%table.products{ 'data-column-preferences-target': "table" } | |||
%table.products{ 'data-column-preferences-target': "table", class: (hide_producer_column?(producer_options) ? 'hide-producer' : '') } |
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 can't see a CSS rule for this class, did you forget to commit 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.
Hmm maybe it's not necessary at all: the column defaults take care of hiding it.
So I guess we don't need this hide_producer_column?
helper.
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 can't see a CSS rule for this class, did you forget to commit it?
It's the existing style:
openfoodnetwork/app/webpacker/css/admin/products_v3.scss
Lines 185 to 195 in 6849155
$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; | |
} | |
} | |
} | |
} |
the column defaults take care of hiding it.
We need it because JS takes time to load and hide the column. Using it in the HTML immediately hides it when necessary.
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.
Thanks for answering my questions, all good.
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 good 👍 , Thanks for adding the comments !
Hey @chahmedejaz , I had a quick test on this one: The column is hidden by default, but the user can still opt-in, if desired. Merging! 🎉 |
83ab959
into
openfoodfoundation:master
What? Why?
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):