-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
textToContours is broken under textAlign(CENTER,CENTER) and use linebreak #8083
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great Perminder! Just left some very minor code format comments, otherwise good to go 🙂
src/type/textCore.js
Outdated
); | ||
// When width is not provided (e.g., fontBounds path), fall back to the widest line. | ||
const maxWidth = boxes.length | ||
? boxes.reduce((m, b) => Math.max(m, b.w || 0), 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super minor, but I think since you provide the initial value of 0 in the reduce, we don't additionally need to have a ternary on boxes.length
; the reduce
will already return 0 here.
test/unit/type/p5.Font.js
Outdated
}); | ||
|
||
test('fontBounds no NaN (multiline + CENTER)', async () => { | ||
const pFont = await myp5.loadFont(fontFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the body of this test might be indented one time too many?
Thanks for your valuable feedback :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for the fix!
Always, a pleasure. |
Resolves #8079