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

- added possibility to write comment tags #87

Merged
merged 5 commits into from
Apr 7, 2021

Conversation

brainbugfix
Copy link
Contributor

I added the possibility to add the comments to the image during encoding as they come from decoding or added manually

- added possibility to write comment tags
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

thanks @brainbugfix! add a round-trip test for this?

types will also need to be updated

lib/encoder.js Outdated

function writeCOM(comments)
{
if (typeof comments === "undefined") return;
Copy link
Collaborator

@patrickhulce patrickhulce Mar 25, 2021

Choose a reason for hiding this comment

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

Suggested change
if (typeof comments === "undefined") return;
if (typeof comments !== 'string') return;

perhaps? but ya know, inside the loop with an array check 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.

This will not work, I am afraid. The type is 'object' as it is an array.

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 sorry, misunderstood your comment. I will change that tomorrow.

- added checks for comments being an array and the content is string
@patrickhulce
Copy link
Collaborator

thanks that change looks great, would you mind tackling...

add a round-trip test for this?
types will also need to be updated

:D

@brainbugfix
Copy link
Contributor Author

I am not sure if I understand what you want me to do. I can add a test, that should not be a big deal but what exactly do you mean with updating the types?

@patrickhulce
Copy link
Collaborator

Oh sorry, I mean update the typescript definitions in index.d.ts at

export declare function encode(imgData: RawImageData<BufferLike>, quality?: number): BufferRet;

It's already a bit out of date but just changing the first encode parameter to RawImageData<BufferLike> & {comments?: string[]} should work?

- updated the typescript
- added a test for writing the comments
@brainbugfix
Copy link
Contributor Author

Did what you asked me for. Can you please double-check the TypeScript because I've never used this before.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

thanks @brainbugfix this is great!

@patrickhulce
Copy link
Collaborator

if all these suggestions are ok with you @brainbugfix I'm ready to merge! :)

@patrickhulce patrickhulce merged commit 13e1ffa into jpeg-js:master Apr 7, 2021
@patrickhulce
Copy link
Collaborator

thanks again :)

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