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

[FIX] sale: configurator option modal change variant #31201

Closed
wants to merge 1 commit into
base: 12.0
from

Conversation

Projects
None yet
4 participants
@kebeclibre
Copy link
Contributor

kebeclibre commented Feb 18, 2019

Part 1: Backend
Changing an option's variant now change the product name and the image

Part 2: Front end
The behavior is the same as in the backend.

For that part though, there is a huge design problem:
the modal dialog has the class .oe_website_sale, so the widget website_sale
reacts to events triggered in the modal.

It is understandable given the history of the module, but a proper refactoring
that will implement inheriting of business behaviors through OdooClass extension/overrides
is necessary in master

OPW 1938217

@robodoo robodoo added the seen 🙂 label Feb 18, 2019

@kebeclibre kebeclibre changed the title 12.0 conf mixin option change lpe [FIX] sale: configurator option modal change variant Feb 18, 2019

@kebeclibre kebeclibre requested a review from seb-odoo Feb 18, 2019

@seb-odoo seb-odoo added the RD label Feb 18, 2019

@robodoo robodoo added the CI 🤖 label Feb 18, 2019

@kebeclibre kebeclibre requested a review from qsm-odoo Feb 18, 2019

addons/sale/static/src/js/product_configurator_modal.js Outdated
@@ -440,7 +439,17 @@ var OptionalProductsModal = Dialog.extend(ServicesMixin, ProductConfiguratorMixi
delete optionalProductsMap[optionId];
}
},
/*

This comment has been minimized.

@qsm-odoo

qsm-odoo Feb 18, 2019

Contributor

/**

addons/website_sale/static/src/js/website_sale.js Outdated
@@ -589,6 +591,24 @@ sAnimations.registry.WebsiteSale = sAnimations.Class.extend(ProductConfiguratorM
_onChangeShippingUseSame: function (ev) {
$('.ship_to_other').toggle(!$(ev.currentTarget).prop('checked'));
},
/*

This comment has been minimized.

@qsm-odoo

qsm-odoo Feb 18, 2019

Contributor

Same

addons/website_sale/static/src/js/website_sale.js Outdated
var nodes = ev.originalEvent.path;
for (var i=nodes.length-1; i >= 0; i--) {
var $node = $(nodes[i]);
if ($node.hasClass('oe_optional_products_modal')) {

This comment has been minimized.

@qsm-odoo

qsm-odoo Feb 18, 2019

Contributor

$(ev.originalEvent.path).hasClass('oe_optional_products_modal') does the same thing as the loop and stuff.

But I am not sure to understand the fix. Should not it be stopPropagation in all cases ?

This comment has been minimized.

@kebeclibre

kebeclibre Feb 19, 2019

Author Contributor

Well, this is the whole problem.
2 handlers are bound to the event:

  • the one on the ProductConfiguratorMixin, instanciated by the ProductConfiguratorModal
  • the one on the widget WebsiteSale
    Those two widgets are not even siblings, as the Modal hooks WebsiteSale (it is the same instance as the one created for the whole page -- ) through its CSS class .oe_website_sale.
    So the fix in WebsiteSale just checks if the event comes from the modal, in which case the WebsiteSale widget shoud not react.
    This is really an interpretation on my part, I'm not really sure which part of websiteSale's behavior the Modal should have....

Hence, stopPropagation maybe for all cases, but here, really the return has relevance

This comment has been minimized.

@qsm-odoo

qsm-odoo Feb 19, 2019

Contributor

If the problem is fixed and @seb-odoo is ok with the fix, ok for me once it is adapted to my comments 😉

addons/sale/static/src/scss/product_configurator.scss Outdated

img.variant_image {
height: 128px;
width: 128px;

This comment has been minimized.

@seb-odoo

seb-odoo Feb 19, 2019

Contributor

We can't hardcode both width and height on the image, because when using the image field (= big size) it might not be a square, so it will distort the image.

What should be done ideally is the parent to be the square, and the image to adapt into it by keeping its ratio (you can check what I did in master with the carousel preview on website_sale).
The problem is that there is no suitable parent for that here.

So either we can find other CSS rules, for example by not forcing a square.
Or we need to change the JS to use the image_medium when showing the image inside the modal.

Finding a way to use image_medium would be the best way, because that's the minimal download size for the user.

@seb-odoo

This comment has been minimized.

Copy link
Contributor

seb-odoo commented Feb 19, 2019

When I open the modal on the website with this branch, I see 6 RPC to get_combination_info_website, that seems a bit excessive.
There are 2 RPC (the same) for each product (the main product, and every option).

@seb-odoo

This comment has been minimized.

Copy link
Contributor

seb-odoo commented Feb 19, 2019

Also, I just noticed, but this is also a problem in 12.0, that the RPC is returning the carousel even when called for the modal, where we don't display the carousel. This has no visual impact, it just makes the RPC a bit slower, so I guess it's not a priority.

@kebeclibre kebeclibre force-pushed the odoo-dev:12.0-conf-mixin-option-change-lpe branch Feb 19, 2019

@robodoo robodoo removed the CI 🤖 label Feb 19, 2019

@kebeclibre kebeclibre force-pushed the odoo-dev:12.0-conf-mixin-option-change-lpe branch Feb 19, 2019

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Feb 19, 2019

@seb-odoo
@qsm-odoo
changes made

I saw the 6 RPC, but at this point I think it is inherent to the design flaws.....

@robodoo robodoo added the CI 🤖 label Feb 19, 2019

@seb-odoo

This comment has been minimized.

Copy link
Contributor

seb-odoo commented Feb 20, 2019

Now that you have figured out the issue and how to solve it, isn't it a quick fix to also not make double rpc?

For the image size fix, I would put the whole field name in a variable instead of just the suffix, that makes it easier to grep.

@kebeclibre kebeclibre force-pushed the odoo-dev:12.0-conf-mixin-option-change-lpe branch Feb 20, 2019

@robodoo robodoo removed the CI 🤖 label Feb 20, 2019

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Feb 20, 2019

@seb-odoo
Alrighty, the duplicate RPC are now covered and avoided.
But, damn, this is ugly

addons/website_sale/static/src/js/website_sale.js Outdated
*/
onChangeVariant: function (ev, data) {
var $originPath = ev.originalEvent && Array.isArray(ev.originalEvent.path) ? $(ev.originalEvent.path) : $();
var $container = data && data.$container ? data.$container : $();;

This comment has been minimized.

@seb-odoo

seb-odoo Feb 20, 2019

Contributor

double ;

@kebeclibre kebeclibre force-pushed the odoo-dev:12.0-conf-mixin-option-change-lpe branch Feb 20, 2019

@seb-odoo

This comment has been minimized.

Copy link
Contributor

seb-odoo commented Feb 20, 2019

Missing modules in commit: sale,sale_management,website_sale

Other than that, I retested what I mentioned and it looks good now. If @qsm-odoo is ok with the last changes, it's ok for me.

I know it's ugly but we have to do with it in 12.0... We'll fix this mess in master after the configurator v2 task.

@robodoo robodoo added the CI 🤖 label Feb 20, 2019

[FIX] sale, sale_management, website_sale: configurator option modal …
…change variant

Part 1: Backend
Changing an option's variant now change the product name and the image

Part 2: Front end
The behavior is the same as in the backend.

For that part though, there is a huge design problem:
the modal dialog has the class .oe_website_sale, so the widget website_sale
reacts to events triggered in the modal.

It is understandable given the history of the module, but a proper refactoring
that will implement inheriting of business behaviors through OdooClass extension/overrides
is necessary in master

OPW 1938217

@kebeclibre kebeclibre force-pushed the odoo-dev:12.0-conf-mixin-option-change-lpe branch to aa1e26f Feb 20, 2019

@robodoo robodoo removed the CI 🤖 label Feb 20, 2019

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Feb 20, 2019

commit message changed to account for all modules change

@robodoo robodoo added the CI 🤖 label Feb 20, 2019

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Mar 7, 2019

@qsm-odoo
could you take a litlle lookeroo ? :D

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Mar 8, 2019

robodoo r+

@robodoo robodoo added the r+ 👌 label Mar 8, 2019

@robodoo robodoo added the merging 👷 label Mar 8, 2019

@robodoo robodoo closed this in ec9fff6 Mar 8, 2019

@robodoo robodoo added merged 🎉 and removed merging 👷 labels Mar 8, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Mar 8, 2019

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.