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

[13.0][IMP]storage_image_product: main image of the variant #115

Merged
merged 1 commit into from
Feb 13, 2022

Conversation

AaronHForgeFlow
Copy link

@OCA-git-bot
Copy link
Contributor

Hi @AaronHForgeFlow! Thank you very much for this contribution. As the addon you are improving does not have a declared maintainer, I take the opportunity to mention that you can consider adopting it. To do so, please read the maintainer role description, and, if interested, create a pull request to add your GitHub login to the maintainers key of the addon manifest.

@simahawk
Copy link
Contributor

simahawk commented Sep 9, 2021

this is similar to #116 but for images right?

@AaronHForgeFlow
Copy link
Author

When the images are attached to the template and not to the variant. In that case in the variant it shows an image without attributes. The fix search first for images with the same attributes than the variant.

Perhaps this is not a common use case as long I am storing all the images in the template instead of storing them in any image individually. If that is the case I will close this.

@sebastienbeau sebastienbeau added this to the 13.0 milestone Oct 6, 2021
@sebastienbeau
Copy link
Member

Hi
I think I understand.
If you put 3 images on a product template in this order

  • a generic image for all product (like a image of the situtation, or an image with all the variant visible)
  • a specific image for the blue product
  • a specific image for the red product

With the existing implementation, the generic one will used as main image for the product template and for all variant.
With your change each variant will try to show the first "specific image" that match for them. So in our case it will a be the blue image for the blue product, the red image for the red product.

I am correct ?

If yes it's seem to be a good idea, can you add a test? If needed i can do it

Thanks

@AaronHForgeFlow
Copy link
Author

If you put 3 images on a product template in this order

a generic image for all product (like a image of the situtation, or an image with all the variant visible)
a specific image for the blue product
a specific image for the red product
With the existing implementation, the generic one will used as main image for the product template and for all variant.
With your change each variant will try to show the first "specific image" that match for them. So in our case it will a be the blue image for the blue product, the red image for the red product.

Yes, exactly :)

I will try to add a test Thanks for the review!

@AaronHForgeFlow AaronHForgeFlow force-pushed the 13.0-imp-storage_image_product branch from 0f4ae9a to e69a3ba Compare October 8, 2021 16:10
@AaronHForgeFlow
Copy link
Author

Test added :)

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@sebastienbeau
Copy link
Member

@AaronHForgeFlow can you add the change propose by alex ? so we can merge this ? Thansk

@AaronHForgeFlow AaronHForgeFlow force-pushed the 13.0-imp-storage_image_product branch from e69a3ba to 64694ea Compare February 11, 2022 13:26
@AaronHForgeFlow
Copy link
Author

@AaronHForgeFlow can you add the change propose by alex ? so we can merge this ? Thansk

Done! Thanks! :)

@sebastienbeau
Copy link
Member

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 13.0-ocabot-merge-pr-115-by-sebastienbeau-bump-major, awaiting test results.

@sebastienbeau
Copy link
Member

Thanks @AaronHForgeFlow for your work !

@OCA-git-bot OCA-git-bot merged commit 21aa51a into OCA:13.0 Feb 13, 2022
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at b228589. 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.

5 participants