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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add drawable kerning #103

Merged
merged 4 commits into from
Jul 6, 2023
Merged

add drawable kerning #103

merged 4 commits into from
Jul 6, 2023

Conversation

tiptop96
Copy link
Contributor

@tiptop96 tiptop96 commented Jul 4, 2023

Hey, this is more a question than a PR so far.

We are adding text to an image using the following code:

image.draw(
    new DrawableGravity(Gravity.Center),
    new DrawableGravity(Gravity.South),
    new DrawableFont("OurCustomFont"),
    new DrawableFontPointSize(100),
    new DrawableFillColor(new MagickColor("white")),
    new DrawableText(0, 190, formatText(titleText))
);

However we have a need to set the text kerning which is not possible.

To work around this we implemented our own DrawableKerning as below, but we figured it might make more sense to get it into the library then keeping it to ourselves.

This is just a draft to give you an idea of what we were going for, we assume it might need tests and more work.

Please let us know if you think this might be a good addition to the lib! 馃コ

@dlemstra
Copy link
Owner

dlemstra commented Jul 4, 2023

This would of course be a good addition to the library. I am adding this step by step based on what I think people would need but I am always happy when a PR comes with something that someone needs. And this is the exact same implementation that I would do when I would add this myself. One tiny remark.... all methods are sorted alphabetically so could you please change that? And this of course needs a test. Maybe something where you check if a pixel has the background color or check if the pixel has the drawing color And do this with two tests, one where you don't set the kerning and one where you do but in both situations you check the same pixel.

p.s. You can run just your own test with npm run test kerning (this matches the filename drawable-kerning.spec.ts)

@tiptop96
Copy link
Contributor Author

tiptop96 commented Jul 5, 2023

Awesome, thanks for the feedback!

We sorted the methods and gave out best shot at adding a test. 馃コ

@tiptop96 tiptop96 marked this pull request as ready for review July 5, 2023 09:18
Copy link
Owner

@dlemstra dlemstra left a comment

Choose a reason for hiding this comment

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

Code looks fine to me and I will merge this if the tests run successfully.

Would it help you if I would publish a new release or are you planning to add more functionality? Or can I wait with the next release?

@dlemstra
Copy link
Owner

dlemstra commented Jul 5, 2023

Whoops 馃榿 You added a new new file so we should add this to the index. I keep forgetting this myself also so I made this part of the build. You can update the index with the following command: npm run update-index.

@hannanilsson
Copy link
Contributor

Hi! We have updated the index now. We are in no hurry to get a new release, we currently have a working workaround in our project with basically the same code that we added in the PR. We will extend our use of image magick in August, so we might come back with more ideas then. 馃槃

@dlemstra dlemstra merged commit d44c76f into dlemstra:main Jul 6, 2023
10 checks passed
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.

None yet

3 participants