Conversation
🦋 Changeset detectedLatest commit: 576dc6e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| "descent": -213, | ||
| "lineGap": 27, | ||
| "unitsPerEm": 1000, | ||
| "xHeight": 0, |
There was a problem hiding this comment.
So this is our first metrics update since moving to fontkitten. These four variants of Helvetica Neue are missing the xHeight metrics, but there is a subtle difference in how fontkitten returns the xHeight compared to fontkit.
In fontkit they fallback to 0 if the OS/2 table doesnt exist (otherwise return the value, e.g. undefined).
In fontkitten they fallback to 0 if the xHeight value is nullish
@delucis feel like the libraries favour missing data over invalid values like 0. Is this something we should fix upstream?
There was a problem hiding this comment.
Ah interesting case. I made the change thinking skipping the 0 case was undesirable, but yeah, I guess no font should really have a 0 x-height (if there is one, I want to see what it looks like! 😄).
We can definitely update fontkitten to match the old behaviour if it’s helpful. (I guess the best fix for this specific case is a valid value in the source font file, but more generically returning undefined instead of a meaningless number is probably better.)
There was a problem hiding this comment.
Opened delucis/fontkitten#105 for this. Would be happy to hear feedback on the approach. fontkit’s logic is kind of a halfway: default to 0 if the table is missing, but otherwise return the value. But this PR just returns undefined when the table is missing, which feels like a more honest reflection of the file data.
It’ll mean types are off: xHeight is technically number | undefined but typed as just number. But that’s something inherited from @types/fontkit and can be fixed in a future major I guess.
No description provided.