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 issue productlist image not shown when child product image empty … #3397

Conversation

ngongoll
Copy link
Contributor

Magento1 with configuable products. If child/variant product don't have a image the product doesn't have a image in category view when category is selected from menu. Add fallback condition for value === 'no_selection'

Copy link
Collaborator

@pkarw pkarw left a comment

Choose a reason for hiding this comment

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

Cool! Please just update the changelog

@pkarw pkarw changed the base branch from master to hotfix/v1.10.1 August 21, 2019 12:43
@pkarw pkarw requested a review from patzick August 21, 2019 12:46
Copy link
Collaborator

@patzick patzick left a comment

Choose a reason for hiding this comment

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

Hey @ngongoll !
Thanks for this fix, I have some suggestions and question regarding childHasImage method :)

export function childHasImage (children) {
let hasImage = false;
children.forEach((child) => {
if (child.image.length == 0 || child.image === 'no_selection') hasImage = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I get this right - could you explain to me why a child has an image if its images have no length or if the image is no_selection? Shouldn't that be the opposite conditions?
also please concider to use includes instead of forEach and provide a default value for children, because with the wrong argument we could have an error here. If you'd add unit tests for this helper method I'd be really grateful :)

let configurableAttributes = product.configurable_options.map(option => option.attribute_code)
configurableChildrenImages = product.configurable_children.map(child =>
({
'src': getThumbnailPath(child.image, config.products.gallery.width, config.products.gallery.height),
'src': getThumbnailPath(((child.image === '' || child.image === 'no_selection') ? product.image : child.image), config.products.gallery.width, config.products.gallery.height),
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could use childHasImage here passing child as array

@@ -567,11 +567,11 @@ export function attributeImages (product) {

export function configurableChildrenImages (product) {
let configurableChildrenImages = []
if (product.configurable_children && product.configurable_children.length > 0) {
if (product.configurable_children && product.configurable_children.length > 0 && this.childHasImage(product.configurable_children)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if childHasImage method would have default value then here it could only look like this

Suggested change
if (product.configurable_children && product.configurable_children.length > 0 && this.childHasImage(product.configurable_children)) {
if (childHasImage(product.configurable_children)) {

@patzick patzick added this to the 1.10.1 milestone Aug 26, 2019
@patzick patzick added the not ready for merge PR is holded. Needs some clarifications or things that need to be finished. label Aug 27, 2019
@pkarw
Copy link
Collaborator

pkarw commented Aug 31, 2019

@ngongoll can you please introduce the requested changes in order to merge this PR?

@patzick patzick removed the not ready for merge PR is holded. Needs some clarifications or things that need to be finished. label Sep 2, 2019
Copy link
Collaborator

@patzick patzick left a comment

Choose a reason for hiding this comment

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

@pkarw, please recheck

@patzick patzick requested a review from pkarw September 2, 2019 12:22
@pkarw
Copy link
Collaborator

pkarw commented Sep 2, 2019

Looks fine!

@patzick patzick merged commit 7363b5c into vuestorefront:hotfix/v1.10.1 Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants