-
-
Notifications
You must be signed in to change notification settings - Fork 492
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
[11.0][NEW] website_sale_product_detail_attribute_image: new module #288
[11.0][NEW] website_sale_product_detail_attribute_image: new module #288
Conversation
I have added free images for demo data from https://www.iconfinder.com/, Where I put this images information? |
bf91388
to
bb20c2c
Compare
Put them on CREDITS.rst |
bb20c2c
to
1764517
Compare
@@ -0,0 +1,2 @@ | |||
This module extends the functionality of website sale module to allow display |
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.
s/to allow/to allow to
@@ -0,0 +1,2 @@ | |||
This module extends the functionality of website sale module to allow display | |||
product attributes images in shop product detail. |
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.
s/product attributes images in shop product detail/images related to product attributes in e-commerce product page
string='Website detail image', | ||
attachment=True, | ||
help='Small-sized image of the attribute for shop online product ' | ||
'detail. It is automatically resized as a 64x64 px image, with ' |
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.
You shouldn't resize this automatically. What about if someone wants it to 128x128? If automatic resize, you have to put a configuration option to select the preferred size.
'detail. It is automatically resized as a 64x64 px image, with ' | ||
'aspect ratio preserved.', | ||
) | ||
website_product_detail_display = fields.Boolean( |
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.
Being homogeneous with other namings, I would call this website_product_detail_image_published
res = super().product( | ||
product, category=category, search=search, **kwargs) | ||
image_attributes = product.attribute_line_ids.filtered( | ||
lambda x: x.attribute_id.website_product_detail_display) |
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.
Isn't better to populate images directly:
attribute_images = product.attribute_line_ids.filtered(
lambda x: x.attribute_id.website_product_detail_display
).mapped('website_product_detail_image')
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 self-answer: you are also displaying the attribute name. Not sure about this being coupled between backend and frontend. It's better to have an extra translate field for website display, but fallbacking to this one if not set.
} | ||
.product-detail-attributes div { | ||
font-size: 85%; | ||
/*line-height: 1;*/ |
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.
Remove this if not used
|
||
class TestProductAttribute(SavepointCase): | ||
at_install = False | ||
post_install = True |
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.
Is this needed?
def test_write_image_for_attribute(self): | ||
self.attribute.website_product_detail_image = self.image | ||
self.assertEqual( | ||
self.attribute.website_product_detail_image, self.image) |
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.
This is not testing actually the resize
<field name="inherit_id" ref="product.attribute_tree_view"/> | ||
<field name="arch" type="xml"> | ||
<field name="create_variant" position="after"> | ||
<field name="website_product_detail_display"/> |
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.
widget switch or boolean?
t-attf-src="/web/image/product.attribute/#{image_attribute.attribute_id.id}/website_product_detail_image"/> | ||
</div> | ||
<t t-foreach="image_attribute.value_ids" t-as="value"> | ||
<div class="text-center"> <span t-field="value.name"/> </div> |
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.
The same about showing these backend string here. You are not mentioning this in the README, nor the label of the attribute.
1764517
to
c0f648e
Compare
Changes done. |
c0f648e
to
f7a33e0
Compare
<field name="inherit_id" ref="product.attribute_tree_view"/> | ||
<field name="arch" type="xml"> | ||
<field name="create_variant" position="after"> | ||
<field name="website_product_detail_image"/> |
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.
Why not uses widget='image' here? size?
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.
is it working on trees?
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.
Work but without resize image :/ perhaps can use: https://github.com/OCA/web/tree/11.0/web_tree_image
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.
Well, maybe too much overhead for this, don't you think? Let Sergio decides.
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 want dependencies of other modules, If the user wants to view the image only has to do click on record, also this model, once it has been configured the user not will coming back to see the images every day.
But I thought so too 😄
website_sale_product_detail_attribute_image/views/product_attribute_views.xml
Outdated
Show resolved
Hide resolved
f7a33e0
to
b491d67
Compare
Changes done |
…an_widget from tree view
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.
LGTM, tested on runbot
This PR has the |
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.
Tested in runbot 👍
…CA#288) * [11.0][NEW] website_sale_product_detail_attribute_image: new module * [11.0][IMP] website_sale_product_detail_attribute_image: Remove boolean_widget from tree view
…CA#288) * [11.0][NEW] website_sale_product_detail_attribute_image: new module * [11.0][IMP] website_sale_product_detail_attribute_image: Remove boolean_widget from tree view
…CA#288) * [11.0][NEW] website_sale_product_detail_attribute_image: new module * [11.0][IMP] website_sale_product_detail_attribute_image: Remove boolean_widget from tree view
…CA#288) * [11.0][NEW] website_sale_product_detail_attribute_image: new module * [11.0][IMP] website_sale_product_detail_attribute_image: Remove boolean_widget from tree view
…CA#288) * [11.0][NEW] website_sale_product_detail_attribute_image: new module * [11.0][IMP] website_sale_product_detail_attribute_image: Remove boolean_widget from tree view
…CA#288) * [11.0][NEW] website_sale_product_detail_attribute_image: new module * [11.0][IMP] website_sale_product_detail_attribute_image: Remove boolean_widget from tree view
…CA#288) * [11.0][NEW] website_sale_product_detail_attribute_image: new module * [11.0][IMP] website_sale_product_detail_attribute_image: Remove boolean_widget from tree view
…CA#288) * [11.0][NEW] website_sale_product_detail_attribute_image: new module * [11.0][IMP] website_sale_product_detail_attribute_image: Remove boolean_widget from tree view
…CA#288) * [11.0][NEW] website_sale_product_detail_attribute_image: new module * [11.0][IMP] website_sale_product_detail_attribute_image: Remove boolean_widget from tree view
…CA#288) * [11.0][NEW] website_sale_product_detail_attribute_image: new module * [11.0][IMP] website_sale_product_detail_attribute_image: Remove boolean_widget from tree view
New module for display attribute image in produts detail shop view
cc @Tecnativa