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

TextMetrics.wordWrap() customization of splitting method #6251

Merged

Conversation

EvidentlyCube
Copy link
Contributor

Description of change

As requested in #6201, made it so that the method used to split a token into characters in TextMetrics.wordWrap() can be customized.

Pre-Merge Checklist
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

…ng a word into characters by TextMetrics.wordWrap()
@codecov-io
Copy link

codecov-io commented Nov 26, 2019

Codecov Report

Merging #6251 into dev will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##              dev   #6251      +/-   ##
=========================================
+ Coverage   76.99%     77%   +<.01%     
=========================================
  Files         202     202              
  Lines       10247   10249       +2     
=========================================
+ Hits         7890    7892       +2     
  Misses       2357    2357
Impacted Files Coverage Δ
packages/text/src/TextMetrics.js 95.21% <100%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0c9e9e...fe64122. Read the comment docs.

@bigtimebuddy
Copy link
Member

Thanks for pulling this together.

I don't have a problem with this functionally, but wondered if we could do something a little simpler for the API.

PIXI.TextMetrics.configuration.wordWrapTokenSplit is rather verbose. Also we don't have other examples in the API that use a configuration object. My preference would be to do something like TextMetrics.wordWrapSplit and specify in the docs that it can be overridden to support emojis (for example). An example of how to support emojis would also be excellent thing to document, since that's the main use-case.

@bigtimebuddy bigtimebuddy added this to the v5.2.1 milestone Dec 4, 2019
@themoonrat
Copy link
Member

For reference, canBreakWords and canBreakChars are other examples of code split out into functions that can be overridden; in those cases to help with custom CJK language support.

@EvidentlyCube
Copy link
Contributor Author

@themoonrat I am happy to change things to better fit the rest of the code :).

If I understand correctly, the way you'd like this to be used would be in essence:

import * as PIXI from 'pixi.js`;

// If this is not done then the default implementation is used
PIXI.TextMetrics.wordWrapSplit = <put your own custom implementation here>;

That sounds fair. My personal preference is a configuration variable, just like the way I implemented it, but I am not here to impose my preferences on others :D. I'll update the PR soonish!

@EvidentlyCube
Copy link
Contributor Author

@themoonrat I've got one question though: you mentioned canBreakWords and canBreakChars but those are marked as @private and thus do not appear in the documentation (at least not when I run npm docs, though I don't see them in the official documentation either). Is it intended or an oversight? I feel like it'd make more sense to expose them in the docs.

And how about changing the line "This method exists to be easily overriden" to something like "Overridable helper method used internally by TextMetrics, exposed to allow customizing the class's behavior"

@themoonrat
Copy link
Member

Yeah, I'd say it's a mistake that they are marked as private! Your changes would be appreciated :)

…is implemented, from a configuration constant to just overriding a function call.
…etrics` and updated their docs slightly
@EvidentlyCube
Copy link
Contributor Author

Pushed the changes - npm run lint and npm run test both pass (with the exceptions of the tests that fail on Windows)

@ivanpopelyshev
Copy link
Collaborator

Do you want to join PixiJS Slack? I can sent an invite to your public email on your site.

@EvidentlyCube
Copy link
Contributor Author

Sure! I was hoping to contribute a bit more, as long as life allows, so that might be helpful.

Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

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

Just some last cleanup bit, otherwise this looks G2G. Thanks!

packages/text/src/TextMetrics.js Show resolved Hide resolved
packages/text/src/TextMetrics.js Outdated Show resolved Hide resolved
@EvidentlyCube
Copy link
Contributor Author

<insert a generic excuse about sleep deprivation/doing fast too quickly/something here>
Fixed those!

@bigtimebuddy bigtimebuddy merged commit 46cf0ba into pixijs:dev Dec 13, 2019
@bigtimebuddy
Copy link
Member

You’re on a serious roll this release @EvidentlyCube!

@EvidentlyCube
Copy link
Contributor Author

Let's see how much more I can fit before the next release 🙃. And if it's around the corner - the one after that!

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

5 participants