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

adjust bottom accents for improved line heights #146

Closed
wants to merge 1 commit into from

Conversation

thundernixon
Copy link
Contributor

This PR makes several small but useful changes to close issue #145, and bring Inter more into the default line height range of comparable UI fonts:

  • Adjusts bottom anchor on /y to move dot to the side of /ydotbelow, as in comparable fonts
    • (The /y glyph has no other accents below, so it doesn’ spill over)
  • Makes /commabelow accent slightly more compact, so it doesn’t go too far past descender
  • Gives /t a bottom anchor to make sure /commabelow accent is visually centered

image

(It also increments the version number to 3.005)

@thundernixon
Copy link
Contributor Author

@rsms I was somewhat conservative on how much I adjusted the /commaaccent, here. As you can see, it's still a bit bigger in Inter than other families. If you want to, it might be worth making it a bit more compact, still. Then again, it's probably not such a bad thing for default line height to be a bit generous, as it will typically make text more readable.

In #145, you mentioned you could take a look at this tonight. Great! Feel free to use this PR as a starting point it you like, or you can start the changes from scratch.

One thing I'm noticing is that, with this PR, the overall default line height is better, but it's less-nice if you take common UI needs into account, such as vertically centering text in a button. For example, here's SF & Inter 3.005, without explicit line height, just auto-centered in a button shape, in Sketch:

image

image

I tend to think that button text looks best if Latin caps are vertically centered, so SF looks better to me in this case.

You can probably fairly safely adjust the winAscent & winDescent values in a way that is slightly separated from actual min/max Y coordinates, but just with the caveat that certain apps may cut off those areas. Still, easy vertical centering is probably of more importance to this family than making sure the /Aring is 100% visible in something like a VS Code terminal (the only context in which I can remember pieces of letters being partially covered).

So, you can do tests on your own, but I would recommend:

  1. Making the /commaaccent slightly more compact
  2. Bringing down the winAscent by a bit (maybe to the top of the /Atilde or something), and seeing whether that vertically centers things in a way that you find suitable

@rsms
Copy link
Owner

rsms commented Mar 30, 2019

Thank you so much for this!

It seems the changes were made on top of a fairly dated master — I ran MergeGlyphs on master vs this, and there are a lot of changes, including missing glyphs.

I'm going to manually introduce these changes onto latest master, and probably include a few small touch-ups to the /y shape while I'm at it.

@thundernixon
Copy link
Contributor Author

It seems the changes were made on top of a fairly dated master — I ran MergeGlyphs on master vs this, and there are a lot of changes, including missing glyphs.

😬Oof, I guess I missed merging in your latest changes – glad you caught that.

The changes were pretty quick, so it shouldn't take you long. Thanks for taking a look!

@thundernixon
Copy link
Contributor Author

(Feel free to close this unless you intend to make the edits within this PR)

rsms added a commit that referenced this pull request Mar 31, 2019
… value: 2728, new descender value: -680

Includes alternate fix to @thundernixon's PR #146
@rsms
Copy link
Owner

rsms commented Mar 31, 2019

Landed. Thanks again :–)

@rsms rsms closed this Mar 31, 2019
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.

2 participants