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

Consider emoji when removing kerning from last character #360

Merged
merged 4 commits into from
May 29, 2019

Conversation

LorDisturbia
Copy link
Contributor

The current implementation of removeKerningFromLastCharacter and restoreKerningOnLastCharacter start from the assumption that the last character range always has length 1. This however is not true for more complex characters, such as emojis.
The consequence of this was a bug in which applying a style to a string that ended with an emoji, while keeping the flag stripTrailingKerning to its default value of true, would result in the emoji not being shown at all.

@ZevEisenberg
Copy link
Collaborator

@LorDisturbia thank you so much for reporting this! I'd love to have it get fixed and stay fixed. Would you mind adding some tests?

@LorDisturbia
Copy link
Contributor Author

LorDisturbia commented Apr 12, 2019

@ZevEisenberg PR updated with the test :)

@LorDisturbia
Copy link
Contributor Author

@ZevEisenberg Let me know if need anything else from me in order to merge the PR. :)

@ZevEisenberg
Copy link
Collaborator

@LorDisturbia will do - I'll need a little time to review it, which I haven't had yet. Is this blocking you? Can you point your project at the branch? I'll try to get it landed when I've got some time. Thanks for adding tests!

@LorDisturbia
Copy link
Contributor Author

@ZevEisenberg No worries, just wanted to check if you needed anything else. We already implemented a workaround in our project. :)

@ZevEisenberg
Copy link
Collaborator

Thanks! We've got a new co-op who is clearing some of the dust off BonMot and getting it building properly in the latest Xcode. Once we do that, we'll hopefully have some time to merge in some of the PRs that have been piling up. I'm excited to land this one!

Copy link
Collaborator

@ZevEisenberg ZevEisenberg left a comment

Choose a reason for hiding this comment

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

@LorDisturbia finally getting around to this. I love how clean and concise your solution is.

I’ve added a couple of comments for things that I’d like to change in the tests. If you don’t mind, I’ve asked @lukeandrews239 to take a crack at addressing them. You’ll still get credit for the bug fix ❤️

Tests/ComposableTests.swift Outdated Show resolved Hide resolved
Tests/ComposableTests.swift Outdated Show resolved Hide resolved
Tests/ComposableTests.swift Outdated Show resolved Hide resolved
@LorDisturbia
Copy link
Contributor Author

I don't mind at all! I'm happy to know my fix is going to be included :)

@ZevEisenberg ZevEisenberg merged commit cf0b6a3 into Rightpoint:master May 29, 2019
@LorDisturbia LorDisturbia deleted the bugfix/emoji-kerning branch May 15, 2020 18:34
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