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 missing aspect ratio magic attribute #170

Merged
merged 1 commit into from Oct 16, 2019

Conversation

@bencrouse
Copy link
Collaborator

bencrouse commented Oct 15, 2019

This magic attribute doesn't need to be calculated, it's the inverse of the aspect ratio we already have.

@bencrouse bencrouse requested review from mttdffy and MMartyn Oct 15, 2019
@@ -44,6 +44,10 @@ def create_from_pipeline!
def placeholder?
true
end

def image_inverse_aspect_ratio
1 / image_aspect_ratio.to_f

This comment has been minimized.

Copy link
@mttdffy

mttdffy Oct 15, 2019

Contributor

should we ever have to worry about #image_aspect_ratio being nil and resulting in a divide by 0 error?

This comment has been minimized.

Copy link
@MMartyn

MMartyn Oct 15, 2019

Contributor

Good call, might as well return 0 if image_aspect_ratio is 0 or nil

This comment has been minimized.

Copy link
@bencrouse

bencrouse Oct 16, 2019

Author Collaborator

I opted for nil, since 0 implies we have an empty image.

This magic attribute doesn't need to be calculated, it's the inverse of the aspect ratio we already have.
@bencrouse bencrouse force-pushed the fix-placeholder-inverse-aspect-ratio branch from 40e051d to 2f7537b Oct 15, 2019
@bencrouse bencrouse marked this pull request as ready for review Oct 16, 2019
@bencrouse bencrouse merged commit 78455a0 into v3.4-stable Oct 16, 2019
6 checks passed
6 checks passed
static_analysis
Details
admin_tests
Details
admin_system_tests
Details
core_tests
Details
storefront_tests
Details
storefront_system_tests
Details
@bencrouse bencrouse deleted the fix-placeholder-inverse-aspect-ratio branch Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.