-
Notifications
You must be signed in to change notification settings - Fork 109
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 compression methods option for passing to jpeg-compress #103
added compression methods option for passing to jpeg-compress #103
Conversation
I've already checked and |
woops, let me check :) |
…od option for jpegrecompress
Ok, I added only String to options, and it's working fine and all tests pass. Let me know if anything, and sorry for the rushed PL yesterday, I didn't even check the contributing text :( |
Much better! Two more things: you don't check the input and as there are only certain allowed values it would be better to limit to them and (following how it is done for other workers) warn about wrong value and use the default; and as the names are not really obvious it would be better to add explanation like in documentation for the project to the values in description. |
… beter explanation for the methods
done! |
Hi Ramiro, |
@@ -3,6 +3,7 @@ | |||
## unreleased | |||
|
|||
* Use quality `0..100` by default in lossy mode of pngquant worker [#77](https://github.com/toy/image_optim/issues/77) [@toy](https://github.com/toy) | |||
* Add `method` option for jpegrecompress, default to `ssim`[#103](https://github.com/toy/image_optim/pull/103) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space before [
and @ramiroaraujo are missing
Hi, long time no see! I'll try to catch up on this probably this weekend |
I added the option to select the compression method, with the same ssim default. Didn't want to introduce many changes, so I opted for the
Array
type option and grabbed the first one, since there wasn't aString
option and didn't want to map the methods with numbersLet me know!