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

fix(lines-wordwrap): support wordbreak and hyphens option on lines method #3

Closed
wants to merge 5 commits into from
Closed

fix(lines-wordwrap): support wordbreak and hyphens option on lines method #3

wants to merge 5 commits into from

Conversation

arnaud-zg
Copy link

Details on issue #2

@coveralls
Copy link

coveralls commented Dec 2, 2017

Coverage Status

Coverage increased (+0.8%) to 84.307% when pulling 78ccfdd on arnaud-zg:fix/lines-support-long-word into 8740058 on bezoerb:master.

@arnaud-zg arnaud-zg changed the title fix(lines-long): support wordwrap option on lines method fix(lines-wordwrap): support wordwrap option on lines method Dec 2, 2017
@coveralls
Copy link

coveralls commented Dec 2, 2017

Coverage Status

Coverage increased (+0.8%) to 84.307% when pulling 3862a09 on arnaud-zg:fix/lines-support-long-word into 8740058 on bezoerb:master.

@bezoerb
Copy link
Owner

bezoerb commented Dec 2, 2017

Thanks @arnaud-zg for working on this :)
I'll take a deeper look at the PR tomorrow.

src/utils.js Outdated
@@ -417,7 +417,30 @@ export function computeLinesDefault({ctx, text, max, wordSpacing, letterSpacing}
}

if ([...line].length !== 0) {
lines.push(line);
if (options.wordwrap) {
line
Copy link
Owner

Choose a reason for hiding this comment

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

The wordwrap option should be considered above before the breakpoint switch...case. You also should only check the last part and not the entire line. (Think about a line with 1000 characters and only the last 10char word is to long)

Copy link
Author

@arnaud-zg arnaud-zg Dec 3, 2017

Choose a reason for hiding this comment

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

Right, I moved it just before the process loop over text parts and compute the lines, and add a breakpoint of type SHY for each new line. About the width of a line, we can't really know how much space it'll be on screen, so I think it's more interesting to measure it with measureText and compare it with max

@coveralls
Copy link

coveralls commented Dec 3, 2017

Coverage Status

Coverage increased (+0.6%) to 84.133% when pulling 5dfa447 on arnaud-zg:fix/lines-support-long-word into 8740058 on bezoerb:master.

@coveralls
Copy link

coveralls commented Dec 5, 2017

Coverage Status

Coverage increased (+0.6%) to 84.133% when pulling b3ef288 on arnaud-zg:fix/lines-support-long-word into 8740058 on bezoerb:master.

@coveralls
Copy link

coveralls commented Dec 5, 2017

Coverage Status

Coverage increased (+0.6%) to 84.133% when pulling 53adbb8 on arnaud-zg:fix/lines-support-long-word into 8740058 on bezoerb:master.

@arnaud-zg
Copy link
Author

It's been a while that my pull request is on pending status, actually I need this feature on my project. It would be great if it can be merged this week.

@bezoerb
Copy link
Owner

bezoerb commented Dec 9, 2017

@arnaud-zg: sorry for taking so long to come back. This implementation may compute wrong values as the browser would not break the words like this (won't add - at the end)
I think it's important that the result reflects the behavior from the browser. Did you know that you can use the word-break: 'break-all' css rule (either in css or overwrite in your call)

 textMetrics(el).lines(text, {}, {'word-break': 'break-all'})
/*
[ 'Craspharetraph',
  'aretragravida.Vi',
  'vamusconsequ',
  'atlacusvelposu',
  'erecongue.Duis',
  'aloremvitaeexa',
  'uctorscelerisqu',
  'enoneuturpis.Ut',
  'imperdietmagn',
  'asitametjustobi',
  'bendumvehicul',
  'a.' ]
*/

Browser:
image

Sorry to not mention this before.
Does this help with your project or do you depend on text-metrics adding the ­ to each line?
You should also keep in mind, that adding the separator will result in grammatical errors

@arnaud-zg
Copy link
Author

arnaud-zg commented Dec 11, 2017

According to word-break and hyphens documentation, I updated my code and added those two option.
Case 1

textMetrics(el).lines(text, {}, {wordBreak: 'break-all'})
/*
[ 'Craspharetraph',
  'aretragravida.Vi',
  'vamusconsequ',
  'atlacusvelposu',
  'erecongue.Duis',
  'aloremvitaeexa',
  'uctorscelerisqu',
  'enoneuturpis.Ut',
  'imperdietmagn',
  'asitametjustobi',
  'bendumvehicul',
  'a.' ]
*/

Case 2

textMetrics(el).lines(text, {}, {hyphens: 'auto'})
/*
[ 'Craspharetraph-',
  'aretragravida.Vi-',
  'vamusconsequ-',
  'atlacusvelposu-',
  'erecongue.Duis-',
  'aloremvitaeexa-',
  'uctorscelerisqu-',
  'enoneuturpis.Ut-',
  'imperdietmagn-',
  'asitametjustobi-',
  'bendumvehicul-',
  'a.' ]
*/

@coveralls
Copy link

coveralls commented Dec 11, 2017

Coverage Status

Coverage increased (+0.9%) to 84.42% when pulling 9f2116a on arnaud-zg:fix/lines-support-long-word into 8740058 on bezoerb:master.

@arnaud-zg arnaud-zg changed the title fix(lines-wordwrap): support wordwrap option on lines method fix(lines-wordbreak): support wordbreak and hyphens option on lines method Dec 11, 2017
@arnaud-zg arnaud-zg changed the title fix(lines-wordbreak): support wordbreak and hyphens option on lines method fix(lines-wordwrap): support wordbreak and hyphens option on lines method Dec 11, 2017
@arnaud-zg
Copy link
Author

Up

let breakPointType = null;

if (options.wordBreak === 'break-all') {
breakPointType = 'BAI';
Copy link
Owner

Choose a reason for hiding this comment

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

'agnasitametjustob-',
'bibendumvehicul-',
'la.'
];
Copy link
Owner

Choose a reason for hiding this comment

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

Hyphens 'auto' behaves different. If you want to add a hyphen at the end of each line you can do

const value = textMetrics(el).lines(text.replace(/(.{1})/g,"$1­"))

@bezoerb
Copy link
Owner

bezoerb commented Dec 12, 2017

Sorry @arnaud-zg, i don't think it's a good idea to add this feature as this won't match the expected behavior. When doing auto hyphens this should be done properly with grammatical correct results.

@arnaud-zg
Copy link
Author

👍

@arnaud-zg arnaud-zg closed this Dec 13, 2017
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