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

Display variant image in cart #2336

Merged
merged 1 commit into from Jan 27, 2015
Merged

Display variant image in cart #2336

merged 1 commit into from Jan 27, 2015

Conversation

ghost
Copy link

@ghost ghost commented Jan 14, 2015

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets #2332
License MIT
Doc PR -

This PR aims to display variant image instead of product master variant image in cart. In case no variant image is available, it displays product master variant image.

if ($this->images->isEmpty()) {
$image = $this->getProduct()->getImage();
} else {
$image = $this->images->first();
Copy link
Member

Choose a reason for hiding this comment

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

I think you can just return here: return $this->images->first();. It will return null in case there is no image.

Copy link
Author

Choose a reason for hiding this comment

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

Yep but I would miss the fallback to the product image. Do you prefer to move the fallback logic elsewhere?

Copy link
Member

Choose a reason for hiding this comment

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

No, I mean just remove else clause and simply return, cause if the first condition is false, it will always return it anyway. :) Like this:

if ($this->images->isEmpty()) {
    return $this->getProduct()->getImage();
}

return $this->images->first();

Copy link
Author

Choose a reason for hiding this comment

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

I didn't get you right ;) Thanks for feedback, this is fixed now.

@stloyd
Copy link
Contributor

stloyd commented Jan 14, 2015

@pjedrzejewski @Arn0d Sounds good for me 👍

@stloyd
Copy link
Contributor

stloyd commented Jan 20, 2015

@pjedrzejewski Feedback?

@ghost
Copy link
Author

ghost commented Jan 27, 2015

Any feedback on this @pjedrzejewski ?

pjedrzejewski pushed a commit that referenced this pull request Jan 27, 2015
Display variant image in cart
@pjedrzejewski pjedrzejewski merged commit 42c9736 into Sylius:master Jan 27, 2015
@pjedrzejewski
Copy link
Member

Perfect, thank you Matthieu! 👍

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 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.

2 participants