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 for files that are rotated after resizing #1

Merged
merged 7 commits into from
Feb 5, 2019

Conversation

adamivancza
Copy link
Contributor

@sagidM
Copy link
Owner

sagidM commented Jan 23, 2019

Hi Adam

Thank you for contributing.
I will look through your suggestion later on.

Copy link
Owner

@sagidM sagidM left a comment

Choose a reason for hiding this comment

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

Because of your contribution, we are going to avoid deprecated methods. Which means s3-resizer is still supported.
I also see that this repository helps people and it really makes me happy.
Thank you again!

index.js Outdated
@@ -45,7 +45,7 @@ exports.handler = function(event, _context, callback) {
return new Promise(() => {}) // the next then-blocks will never be executed
}

return img.withoutEnlargement().toBuffer();
return img.withoutEnlargement().rotate().toBuffer();
Copy link
Owner

@sagidM sagidM Jan 25, 2019

Choose a reason for hiding this comment

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

v0.21 Changelog says that .withoutEnlargement, .max, and .min are deprecated.
It seems we need to change .resize method above on :L30 to
.resize(..., ..., {withoutEnlargement: true, fit}).
Where fit is null | 'inside' | 'outside'. I.e.

var fit;
switch (func) {
  case 'max': fit = 'inside'; break;
  case 'min': fit = 'outside'; break;
  case null: fit = null; break;
  default: // ...
}

And could you please explain why should we have calling .rotate() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing - will do those changes. rotate function should rotate the image according to EXIF info: https://sharp.pixelplumbing.com/en/stable/api-operation/#rotate. the image I've attached is from an Android device - when you view the original image it's in portrait. but when I've resized it with your library it's resized to a landscape image. to fix this I've added the portrait call - that will rotate the image back to original position using EXIF info.

README.md Outdated
@@ -147,7 +147,7 @@ or
* * Go to lambda, click on **Test**, and paste this json:
```json
{
"queryStringParameters": {"path": __YOUR_IMAGE_PATH_WITH_SIZE_PREFIX__}
"queryStringParameters": {"path": "__YOUR_IMAGE_PATH_WITH_SIZE_PREFIX__"}
Copy link
Owner

@sagidM sagidM Jan 25, 2019

Choose a reason for hiding this comment

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

I intentionally have written the value that way.
Firstly, it's red and captures a look immediately.
Secondly, even if a user copies and pastes it as it is, it is invalid json, so it is going to throw an error.

However, in case you are able to give a good explanation why the changed code is better, we can continue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah I see - it caused me an invalid JSON as well that's why I wanted to fix it. But your reasoning makes sense! I'll revert that change :)

@adamivancza
Copy link
Contributor Author

updated my PR @sagidM

@sagidM
Copy link
Owner

sagidM commented Feb 2, 2019

It seems you've forgotten to use func, removing case clause =)
https://github.com/sagidM/s3-resizer/blob/f49db24ef16d436d9cedfe3a4e3fa995a5982eb6/index.js
(options must contain fit; max => 'inside', min => 'outside')

Since in one-line object there's no spaces, it contrasts using spaces in one place.
{myObject: '!'}, { yourObject: '!' }


By the way, I don't remember why I chose that name; would not it be better to rename func => action?
What do you think?

@adamivancza
Copy link
Contributor Author

ohhh right - I've added that missing handler for func. Yeah that was a good shout to rename func to action - definitely makes more sense that way :)

@sagidM sagidM merged commit 39d4cef into sagidM:master Feb 5, 2019
@sagidM
Copy link
Owner

sagidM commented Feb 5, 2019

Okay, we have done here.
I will update setting up instructions and upload a new release later on.
Thank you very much Adam!

sagidM added a commit that referenced this pull request Feb 5, 2019
@sagidM
Copy link
Owner

sagidM commented Feb 5, 2019

Did that by mistake...

@adamivancza
Copy link
Contributor Author

Awesome - thanks for all the suggestions on the PR :)

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