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

Added logic to prevent upscaling product images #495

Merged
merged 2 commits into from May 19, 2016

Conversation

markguinn
Copy link

No description provided.

@markguinn
Copy link
Author

@bummzack - could you give this a quick review? Do you see any problems with doing this?

return $this->owner;
}

$dim = explode('x', $this->owner->getDimensions());
Copy link
Collaborator

@bummzack bummzack May 10, 2016

Choose a reason for hiding this comment

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

Why not use $this->owner->getWidth() and $this->owner->getHeight()?

Copy link
Author

Choose a reason for hiding this comment

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

The main reason is to not have it run the calculations twice. Last time I checked the dimensions were not cached in any way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the string concatenation/explode via getDimensions really faster than two individual calls for getHeight and getWidth?

I'd rather have better readable and more stable code (what if the joining string changes from x to × or something else?). Using getWidth and getHeight is also in line with what is being used in the framework code (look at the usage of getWidth and getHeight vs. getDimensions)

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense to me.

@markguinn markguinn merged commit 4b49f44 into silvershop:2.0 May 19, 2016
@markguinn markguinn deleted the patch-imagescale branch May 19, 2016 17:34
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.

None yet

2 participants