Skip to content
This repository was archived by the owner on Feb 6, 2022. It is now read-only.

Rewrite to proper logic #70

Merged
merged 4 commits into from
Jul 3, 2016
Merged

Rewrite to proper logic #70

merged 4 commits into from
Jul 3, 2016

Conversation

rogierslag
Copy link
Member

@rogierslag rogierslag commented Jun 28, 2016

@TiddoLangerak @wouter-willems @joostverdoorn Can I haz a first 👀 ? Did a small Tiddo and wanted to rewrite this to proper logic. This way the code does not hurt my eyes anymore.

  • Use async await instead of callback hell
  • Proper modularization
  • Proper passthrough of information
  • Much better imagemagick logic
  • Cropping on upload works

The diff is terrible though, so you might be better off checking the branch itself.

@rogierslag
Copy link
Member Author

// When cropping, we size the image first to fix completely within the bounding box
// Then we crop the requested size, and center to the center of the image
const crop = async(client, params) => {
client = client.resize(params.width, params.height, '^');
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is that ^ ? needs a comment

}
}

// Check if the minimum is ste
Copy link
Collaborator

Choose a reason for hiding this comment

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

dafuq is ste ?

@wouter-willems
Copy link
Collaborator

nice dude!
Test thoroughly! Besides my comments:
👍

result.type = req.params.format.toLowerCase();
result.mime = getMimeFromExtension(result.type);
}
if (req.query.fit && ALLOWED_FITS.includes(req.query.fit)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

needs a .toLowerCase as well

@rogierslag
Copy link
Member Author

rogierslag commented Jun 29, 2016

Todo:

  • Ensure tests pass
  • Test thoroughly
  • Add the blur migration

});
const S3 = new AWS.S3();

const uploadPromise = (client, params) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

uploadPromise isn't a promise. I'd just call it upload.

@TiddoLangerak
Copy link
Collaborator

Nice work! I have quite a few comments, but all are minor. So definitely 👍

@rogierslag rogierslag merged commit d23d7d0 into develop Jul 3, 2016
@rogierslag rogierslag deleted the rewrite branch July 3, 2016 09:37
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.

3 participants