Skip to content
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

[IMP] ecommerce: custom ribbons #36429

Closed

Conversation

@kea14
Copy link
Contributor

commented Sep 4, 2019

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@C3POdoo C3POdoo added the RD label Sep 4, 2019
Copy link
Contributor

left a comment

@kea14 Come on... No JS was required at all here... The spec is clear: transform the "Image Full" product.style in a customize_show view which enables the related style for all products. So... to split the step1 even further...

  1. Remove the "Image - Full" product.style
  2. Create a customize_show view with the same name
  3. When enabled, that customize_show view adds the related style for all products during the rendering of the shop grid

This is a 1 hour task. It is just removing and moving stuff... probably more removing than moving...

How would you defend your work here ? You use JS to flag a customize option when the key contains a hard-coded string, you include the customize menu to allow enabling a product.style (that you target by name...) for all products (which I don't even know how it could work since you don't know the product ids... and what if we create a product afterwards?), ... also why would it be step 1 of the ribbon task ? Did you try to understand how the product.style model work doing this ?

@kea14

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

@qsm-odoo Thanks, I get it, it was pretty obvious indeed

@kea14 kea14 force-pushed the odoo-dev:master-ecommerce-custom-ribbons-aul branch from 4e5010c to 7c1b8ab Sep 5, 2019
@kea14

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

@qsm-odoo So I guess this is something like that for step1, right ?

@robodoo robodoo removed the CI 🤖 label Sep 5, 2019
addons/website_sale/views/templates.xml Outdated Show resolved Hide resolved
@kea14 kea14 force-pushed the odoo-dev:master-ecommerce-custom-ribbons-aul branch from 7c1b8ab to dde99b2 Sep 23, 2019
addons/website_sale/views/templates.xml Outdated Show resolved Hide resolved
addons/website_sale/views/templates.xml Outdated Show resolved Hide resolved
@kea14 kea14 force-pushed the odoo-dev:master-ecommerce-custom-ribbons-aul branch from dde99b2 to c2d3395 Sep 23, 2019
@robodoo robodoo added the CI 🤖 label Sep 23, 2019
@kea14 kea14 force-pushed the odoo-dev:master-ecommerce-custom-ribbons-aul branch from c2d3395 to 0fbd350 Sep 24, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Sep 24, 2019
@kea14 kea14 force-pushed the odoo-dev:master-ecommerce-custom-ribbons-aul branch from 0fbd350 to 39fc561 Sep 25, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Sep 25, 2019
@kea14 kea14 force-pushed the odoo-dev:master-ecommerce-custom-ribbons-aul branch from 39fc561 to cf22959 Sep 27, 2019
@robodoo robodoo removed the CI 🤖 label Sep 27, 2019
@kea14 kea14 force-pushed the odoo-dev:master-ecommerce-custom-ribbons-aul branch from cf22959 to 22edb83 Sep 30, 2019
@robodoo robodoo added the CI 🤖 label Sep 30, 2019
@kea14 kea14 force-pushed the odoo-dev:master-ecommerce-custom-ribbons-aul branch from 22edb83 to ec9ead1 Oct 1, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Oct 1, 2019
@kea14 kea14 force-pushed the odoo-dev:master-ecommerce-custom-ribbons-aul branch from ec9ead1 to 548d8cd Oct 7, 2019
@robodoo robodoo removed the CI 🤖 label Oct 7, 2019
@kea14 kea14 force-pushed the odoo-dev:master-ecommerce-custom-ribbons-aul branch from 548d8cd to 2b7cf3c Oct 9, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Oct 9, 2019
@kea14 kea14 force-pushed the odoo-dev:master-ecommerce-custom-ribbons-aul branch from 2b7cf3c to 5e88286 Oct 10, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Oct 10, 2019
@kea14 kea14 force-pushed the odoo-dev:master-ecommerce-custom-ribbons-aul branch from 5e88286 to 507f53e Oct 11, 2019
@robodoo robodoo removed the CI 🤖 label Oct 11, 2019
@kea14 kea14 force-pushed the odoo-dev:master-ecommerce-custom-ribbons-aul branch from 507f53e to e21690b Oct 11, 2019
@kea14

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2019

In a few words, this task seems almost over. I'll re-reading everything and re-do some tests on Monday.

@robodoo robodoo added the CI 🤖 label Oct 11, 2019
@qsm-odoo

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

Why not but a bit afraid of the code, I think the task can be done in 30lines if we take advantage of the backend... you should speak of the specs with the team during the development... :|

@kea14

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2019

Let's discuss about it when you've some time 😄

kea14 added 3 commits Sep 23, 2019
This commit transforms the "Image Full" option which is per product in a
customize_show view which enables the same style for all products at the
same time.

task-1981170
*: web_editor

This commit brings the possibility to create custom ribbons to put on
products on the shop page.

task-1981170
@kea14 kea14 force-pushed the odoo-dev:master-ecommerce-custom-ribbons-aul branch from e21690b to 8b6e878 Oct 14, 2019
@robodoo robodoo removed the CI 🤖 label Oct 14, 2019
resizeRibbons: function ($el) {
var self = this;
if ($el) {
self.$ribbonWrapper = $($el);
_resizeRibbon();
} else {
_.each($('div.ribbon-wrapper:visible'), function ($ribbonWrapper) {
self.$ribbonWrapper = $($ribbonWrapper);
_resizeRibbon();
});
}

function _resizeRibbon() {
var $ribbon = self.$ribbonWrapper.find('a.ribbon');
var ribbonLength = $ribbon.text().length;
if (ribbonLength >= 10) {
var size = '145px';
self.$ribbonWrapper.css({
'width': size,
'height': size,
});
$ribbon.css({
'top': '25px',
'width': size,
'font-size': '85%',
});
}
}
},
Comment for lines +228  – +256

This comment has been minimized.

Copy link
@kea14

kea14 Oct 14, 2019

Author Contributor

This is very crappy 💩 but I don't have better idea for this part of the specs:

"we need a smart ribbon. If the word is longer, move down the ribbon to have everything visible."

I need your expertise on that @qsm-odoo . Do you have a better idea ? Maybe only with CSS ?

@robodoo robodoo added the CI 🤖 label Oct 14, 2019
if not active:
product.write({'website_style_ids': [(4, style.id)]})
product.write({'website_style_id': style.id})

This comment has been minimized.

Copy link
@JKE-be

JKE-be Oct 14, 2019

Contributor

==>
:/

remove = [] -> not more used
style = request.env['product.style'].browse(style_id) -> useless, you just use the id later...
if ...: val = True if not val: xxx => or just an if / else

=>

new_value = False if product.website_style_id.id == style_id else style_id
product.write({'website_style_id': new_value})
'color': color
})
return new_ribbon.id

@http.route(['/shop/change_styles'], type='json', auth='user')
def change_styles(self, id, style_id):
product = request.env['product.template'].browse(id)

This comment has been minimized.

Copy link
@JKE-be

JKE-be Oct 14, 2019

Contributor

should we handle ribbon on product.product instead of template ?

@@ -6,7 +6,7 @@ access_product_category_pos_manager,product.public.category manager,model_produc
access_product_public_category_public,product.category.public,model_product_public_category,,1,0,0,0
access_product_pricelist_public,product.pricelist.public,product.model_product_pricelist,,1,0,0,0
access_product_pricelist_item_public,product.pricelist.item.public,product.model_product_pricelist_item,,1,0,0,0
access_product_style,product.style.public,website_sale.model_product_style,,1,0,0,0
access_product_style,product.style.public,website_sale.model_product_style,,1,1,1,1

This comment has been minimized.

Copy link
@JKE-be

JKE-be Oct 14, 2019

Contributor

🙄

def add_or_edit_style(self, name, color, id=None, edit=False):
product_style = request.env['product.style']
html_class = 'oe_ribbon_' + name.replace(' ', '_')
if edit:

This comment has been minimized.

Copy link
@JKE-be

JKE-be Oct 14, 2019

Contributor
vals = {
    'name': name,
    'html_class': html_class,
    'color': color
}

if edit:
   x.write(vals)
else:
   x.create(vals)

when you see duplicated lines, is rarely a good case :/

This comment has been minimized.

Copy link
@JKE-be

JKE-be Oct 14, 2019

Contributor

can't we suppose that
no id == new
id == edit

instead to have id + edit ?

Copy link
Contributor

left a comment

Some changes to make.

JKE and SBU think a frontend edition is good but we at least need to use the whole colorpicker not just random colors. In a separate commit in this PR, try to make this:
image
a generic widget which can be used for your purpose and which contains a "custom" button to open the colorpicker dialog (same as in the top bar of the editor).

@@ -140,6 +140,7 @@ $o-wsale-products-layout-grid-gutter-width: min($grid-gutter-width / 2, $o-wsale
height: 88px;
z-index: 5;
overflow: hidden;
pointer-events: none;

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Oct 14, 2019

Contributor

Why do we need this?

@@ -71,7 +71,7 @@ def process(self, products, ppg=20, ppr=4):
self.table[(pos // ppr) + y2][(pos % ppr) + x2] = False
self.table[pos // ppr][pos % ppr] = {
'product': p, 'x': x, 'y': y,
'class': " ".join(x.html_class for x in p.website_style_ids if x.html_class)
'class': " ".join(x.html_class for x in p.website_style_id if x.html_class)

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Oct 14, 2019

Contributor

If this really became _id instead of _ids, I guess there is only one...

@@ -1054,25 +1054,40 @@ def add_product(self, name=None, category=None, **post):
})
return "%s?enable_editor=1" % product.product_tmpl_id.website_url

@http.route(['/shop/add_or_edit_style'], type='json', auth='user', methods=['POST'], website=True)
def add_or_edit_style(self, name, color, id=None, edit=False):

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Oct 14, 2019

Contributor

Why do you need the edit parameter if you got the ID ?

html_class = 'oe_ribbon_' + name.replace(' ', '_')
if edit:
product_style.browse(id).write({
'name': name,
'html_class': html_class,
'color': color
})
return id
else:
new_ribbon = product_style.create({
'name': name,
'html_class': html_class,
'color': color
})
return new_ribbon.id
Comment for lines +1060  – +1074

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Oct 14, 2019

Contributor

This whole code can be factorized, but why do you need it at all? Just do a write or create RPC in javascript-only.

this._rpc({
    model: ...
    method: ...
})....

(check examples in the code)

@http.route(['/shop/change_styles'], type='json', auth='user')
def change_styles(self, id, style_id):
product = request.env['product.template'].browse(id)

remove = []
active = False
style_id = int(style_id)
for style in product.website_style_ids:
if style.id == style_id:
remove.append(style.id)
active = True
break
if product.website_style_id.id == style_id:
product.write({'website_style_id': False})
active = True

style = request.env['product.style'].browse(style_id)

if remove:
product.write({'website_style_ids': [(3, rid) for rid in remove]})
if not active:
product.write({'website_style_ids': [(4, style.id)]})
product.write({'website_style_id': style.id})

return not active
Comment for lines 1076  – 1092

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Oct 14, 2019

Contributor

This method can be written in 3 or 4 lines... maybe even in javascript only?

@@ -6,7 +6,7 @@ access_product_category_pos_manager,product.public.category manager,model_produc
access_product_public_category_public,product.category.public,model_product_public_category,,1,0,0,0
access_product_pricelist_public,product.pricelist.public,product.model_product_pricelist,,1,0,0,0
access_product_pricelist_item_public,product.pricelist.item.public,product.model_product_pricelist_item,,1,0,0,0
access_product_style,product.style.public,website_sale.model_product_style,,1,0,0,0
access_product_style,product.style.public,website_sale.model_product_style,,1,1,1,1

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Oct 14, 2019

Contributor

...? So a website visitor can change the colors of your ribbons ?

route: '/shop/change_styles',
params: {
'id': this.productTemplateID,
'id': self.productTemplateID,

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Oct 14, 2019

Contributor

Why ?

'style_id': value,
},
}).then(function () {

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Oct 14, 2019

Contributor

Use ES6 javascript to avoid the use of self (same everywhere)

'<div class="input-group mb-3 mt-3 o_input_add_edit_ribbon d-none">\
<input type="text" class="form-control o_ribbon_name" placeholder="Ribbon name">\
<div class="input-group-append">\
<button class="btn btn-info o_wsale_soptions_change_color_ribbon" type="button"><i class="fa fa-paint-brush"></i></button>\
<button class="btn btn-primary o_wsale_soptions_validate_ribbon" data-color="' + self.DEFAULT_RIBBON_COLOR + '" data-edit="false" type="button"><i class="fa fa-check"></i></button>\
<button class="btn btn-danger o_wsale_soptions_cancel_ribbon" type="button"><i class="fa fa-remove"></i></button>\
</div>\
</div>'
Comment for lines +297  – +304

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Oct 14, 2019

Contributor

qweb...

@@ -149,7 +149,7 @@
<div class="card-body p-1 oe_product_image">
<input type="hidden" name="csrf_token" t-att-value="request.csrf_token()" />
<div class="ribbon-wrapper">
<a href="#" role="button" class="ribbon btn btn-danger">Sale</a>
<a href="#" role="button" class="ribbon btn" t-attf-style="background-color:#{product.website_style_id.color};"><t t-esc="product.website_style_id.name"/></a>

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Oct 14, 2019

Contributor

Why is not this crashing if there is no website_style_id ?

@kea14 kea14 closed this Oct 15, 2019
@robodoo robodoo added closed 💔 and removed CI 🤖 labels Oct 15, 2019
@qsm-odoo qsm-odoo deleted the odoo-dev:master-ecommerce-custom-ribbons-aul branch Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.