Skip to content
This repository was archived by the owner on May 25, 2023. It is now read-only.

Conversation

jbostoen
Copy link
Contributor

@jbostoen jbostoen commented Nov 14, 2017

Sorry, I'm still new to Github and couldn't change my previous request, so I forked the project and created this commit.

Based on a use case I've encountered:

  • sometimes max width/height was ignored and a copy of the 'main' image was returned; rather than a resized version.
  • max_width = 0, max_height = 0 are Imagick resizeImage options. They were ignored in UploadHandler because empty() returns true for '0'. Using isset() now.
  • added more comments in the 'thumbnail' version about options.
  • added more comments about inheriting of settings (and made it more clear no_cache might be essential and is NOT inherited)

Implemented changes based upon comments.

jbostoen added 4 commits November 14, 2017 20:45
- fixed indentation
- fixed spelling error
- restored a commented-out array as example of 'another version' of the image
- moved max_width, max_height comments
Improved code based upon comments.
@jbostoen jbostoen changed the title Jbostoen patch 2 Update UploadHandler.php (fixed) Nov 14, 2017
@blueimp
Copy link
Owner

blueimp commented Nov 19, 2017

No worries, I don't expect every contributor to be intricately familiar with GitHub.
I try my best to support any contributor, no matter the experience level.

If you want to add changes to an existing pull request, you can simply push additional commits to the branch where you started the pull request from.

Copy link
Owner

@blueimp blueimp left a comment

Choose a reason for hiding this comment

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

Just one small change request, else I this is ready to be merged.

'max_width' => 80,
'max_height' => 80
),
*/
Copy link
Owner

Choose a reason for hiding this comment

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

I still prefer the existing medium example, but I prefer your more descriptive comment line.

Copy link
Contributor Author

@jbostoen jbostoen left a comment

Choose a reason for hiding this comment

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

Changed example to 'medium' again, with different width/height than thumbnail.

- example renamed to 'medium', different width/height than thumbnail
'max_width' => 200,
'max_height' => 160
),
*/
Copy link
Owner

Choose a reason for hiding this comment

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

Again, please keep the original dimensions for the example.
Changing sample dimensions should only be done if there's a good reason for it.

@blueimp blueimp merged commit 25c536f into blueimp:master Nov 20, 2017
@blueimp
Copy link
Owner

blueimp commented Nov 20, 2017

Thanks @jbostoen !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants