-
Notifications
You must be signed in to change notification settings - Fork 13
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
Return a font's example text #12
Conversation
@pascalw Is this worth adding a test for? |
Yeah I would add a test for this. Should take a few minutes only and could prevent surprises down the road. |
Need to add a test font with a custom text first, these are quite rare 😆 |
Ok well if it that's difficult I'm not opposed to not covering it. |
@pascalw Added a name table entry to our test font anyway. Then found out Font.js does indeed return strings intermixed with
|
Nice catch @RoelN ! |
test/Fondue.test.js
Outdated
@@ -6,8 +6,10 @@ const otfFont = async () => { | |||
); | |||
}; | |||
|
|||
const ttfFont = async () => { | |||
return await loadFondue("./test/fixtures/letterA/letterA.ttf"); | |||
const WhatCanThisFontDo = async () => { |
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 hurts readability of the tests a bit IMO. All the fixture helpers refer to the kind of font, which is usually important for the tests. WhatCanThisFontDo
doesn't say anything about the kind of font?
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.
The idea was to put several testable things inside this font, ranging from metadata to glyph count, and in the future OT features, etc. So I'd like to emphasize it's a versatile test font. Perhaps simply WFTestFont
? The readme contains details about exactly what this font does and how it can be used for testing?
I'll change it to that for now (agreed that WhatCanThisFontDo
is more cute than informative) and we can improve it during future changes.
Based on #9, merge that first