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

change dpi buffer #15

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

dungmv
Copy link

@dungmv dungmv commented Apr 27, 2020

change dpi from a buffer source

dist/index.js Outdated
@@ -51,6 +52,13 @@ var _H = 'H'.charCodeAt(0);
var _Y = 'Y'.charCodeAt(0);
var _S = 's'.charCodeAt(0);

function changeDpiBuffer(format, blob, dpi) {
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 the blob argument here? a blob or a buffer?

Copy link
Author

Choose a reason for hiding this comment

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

sorry, the variable name is confusing, it should be buffer

Copy link
Collaborator

Choose a reason for hiding this comment

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

So a couple of suggestion

  1. rename it buffer

  2. With this addition the users have to specify the format by themselves. That is fine, but they have to know how to do it. Docs will be a way for sure, but also adding to the exports the current 2 supported formats is an help, as you did for the new function. so that they can just reference the 2 formtted strings image/jpeg and image/png.

@asturur
Copy link
Collaborator

asturur commented Apr 27, 2020

Do not forget to also add a test please.

@asturur
Copy link
Collaborator

asturur commented Apr 27, 2020

Ok sorry, my bad, we have already a PR doing this:
#8
that includes types detection and test.
Did you try that code? any reason why you feel like this version is better?

@dungmv
Copy link
Author

dungmv commented Apr 27, 2020

Ok sorry, my bad, we have already a PR doing this:
#8
that includes types detection and test.
Did you try that code? any reason why you feel like this version is better?

Sorry, I don't see it. So you can reject this pr

@asturur
Copy link
Collaborator

asturur commented Apr 27, 2020

I do not need to reject it, i wonder if you tried it and if it fits your need. If it does, i could merge that and we could try to polish what is missing if you want to help anyway with a contribution.

@dungmv
Copy link
Author

dungmv commented Apr 27, 2020

I do not need to reject it, i wonder if you tried it and if it fits your need. If it does, i could merge that and we could try to polish what is missing if you want to help anyway with a contribution.

My project needs to save an image with 300 dpi from nodejs with Jipm. Jimp can get buffer and save to file, but it can't set dpi. so I need this feature for my project.
btw, Jimp already defined image type, therefore, I don't need auto-detect format of the image

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