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

Moved imagemin out and provided a way to inject it if you want it #39

Merged
merged 2 commits into from
May 16, 2018

Conversation

boutell
Copy link
Member

@boutell boutell commented May 16, 2018

See #38 for the rationale. See also the uploadfs-imagemin-test repo, a version of sample.js which shows it works.

@boutell boutell requested a review from agilbert May 16, 2018 19:34
Copy link
Member

@agilbert agilbert left a comment

Choose a reason for hiding this comment

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

I made a small comment on the docs changes and reviewed the code. There are many significant changes to remove the dependency on imagemin which at a high level seem good, but you may want a second reviewer with more direct knowledge of the guts of this.

@@ -28,11 +28,13 @@ You need:

* Patience, to wait for [Jimp](https://github.com/oliver-moran/jimp) to convert your images; or [Imagemagick](http://www.imagemagick.org/script/index.php), if you want much better speed and full animated GIF support; OR, on Macs, the very fast [imagecrunch](http://github.com/punkave/imagecrunch) utility. You can also write a backend for something else (look at `imagemagick.js`, `imagecrunch.js`, and `jimp.js` for examples; just supply an object with the same methods, you don't have to supply a factory function).

* If you want GIF support: [Imagemagick](http://www.imagemagick.org/script/index.php) or (Mac-specific) [imagecrunch](http://github.com/punkave/imagecrunch). `jimp` requires no installation of system packages, but it does not yet support GIF.
Copy link
Member

Choose a reason for hiding this comment

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

A little confused if this is duplicating some of the information in the preceding paragraph?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I will give that text one more pass. What changed is that we realized there's no GIF support at all in the default jimp backend you get if you don't have imagemagick installed. Funny that no one complained...

@boutell
Copy link
Member Author

boutell commented May 16, 2018

Thanks. I have confidence in the tests I wrote plus the test app I developed, so I'm going to merge.

@boutell boutell closed this May 16, 2018
@boutell boutell reopened this May 16, 2018
@boutell boutell merged commit 4ef2fa8 into master May 16, 2018
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