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

[16.0][ADD] New module website sale product image sample #827

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

manuelcalerosolis
Copy link
Contributor

This module allows you to select the product variants by displaying a thumbnail image.

Quick sample

image

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addon @manuelcalerosolis Some comments

  • I couldn't set the attribute image, is the view extension missing?

@@ -0,0 +1,5 @@
* For the correct functioning of the module I had to rewrite .css_attribute_color, impacting the way of representing the selection by colors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding an extra class that overrides those styles only for the image thumbnails?

@manuelcalerosolis
Copy link
Contributor Author

The image is obtained from the variant (product.product), it is so by customer requirements.

Buscar esto en Google

@chienandalu
Copy link
Member

The image is obtained from the variant (product.product), it is so by customer requirements.

Ah, that makes sense :)

About the styles, did you explore adding an extra class so you don't need to change the base one?

@manuelcalerosolis
Copy link
Contributor Author

@chienandalu

I tried creating a new css .css_attribute_product_image but the thumbnail image doesn't stay selected.

The problem is this line

t-attf-class="css_attribute_product_image #{'active' if ptav in combination else ''}"

I think the "combination" variable doesn't work with another css selector

peek_no_select

@chienandalu
Copy link
Member

@chienandalu

I tried creating a new css .css_attribute_product_image but the thumbnail image doesn't stay selected.

Yes, that's because the js logic uses that class as a selector. I meant an additional class that would add up/override the other styles. Something like this:

t-attf-class="css_attribute_color css_attribute_product_image #{'active' if ptav in combination else ''} #{'custom_value' if ptav.is_custom else ''}"

@manuelcalerosolis
Copy link
Contributor Author

@chienandalu

Thanks for your help, I have created a new .css and added it to the old one as you indicated, now it works correctly.

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thanks :)

@manuelcalerosolis manuelcalerosolis changed the title [16.0][ADD] New module website sale product image sample #2590 [16.0][ADD] New module website sale product image sample Aug 18, 2023
Copy link
Member

@ioans73 ioans73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional review 👍🏻

@HaraldPanten
Copy link

@pedrobaeza Can this one be merged? THX

@pedrobaeza pedrobaeza added this to the 16.0 milestone Sep 20, 2023
@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-827-by-pedrobaeza-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Sep 20, 2023
Signed-off-by pedrobaeza
@OCA-git-bot
Copy link
Contributor

@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-827-by-pedrobaeza-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-827-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 34c51b6 into OCA:16.0 Sep 20, 2023
4 of 6 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at f22fd97. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants