Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add refresh fonts #1030
Add refresh fonts #1030
Changes from all commits
542e685
707ac8f
4831c54
7c541fa
95ab4c4
e993c57
f04f1e5
98cd8d2
0e4ca0e
873922c
a552315
67f9e12
cdea8f5
957fa82
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
mozilla/bedrock#15790 (comment)
just noticed this comment on another PR, maybe best to use
none
as default here as wellThere 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.
🔥
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.
Unadjusted

Adjusted

Helvetica Neue a tougher fallback font to adjust for. Defaults from https://highperformancewebfonts.com/tools/fafofal/ seemed equally off and manual adjustment on top of that hasn't been a huge improvement
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.
I don't think we need to stick with Helvetica Neue if we can get a better result with something else. Since this is a serif maybe Georgia? It seems to have good support across OSs.
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.
Definitely worth a try!
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.
Suggested adjustment

Suggested + Manual adjustments
This would mean our Headline stack becomes "'Mozilla Headline', 'Georgia', Arial, X-LocaleSpecific, sans-serif" in protocol-tokens package. I like the idea of having a serif fallback in this case, but I'm still on the fence about whether there's a meaningful improvement in fallback sizing compared to Helvetica Neue.
Slight preference is leaving Helvetica Neue for now, getting everything hooked into bedrock and doing a follow up refinement update if needed.
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.
👍
Okay, thanks for looking at it.
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.
Unadjusted

Adjusted

This file was deleted.
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.
There's a repo https://github.com/mozilla/mozilla-type-family — however it still only houses pre-release versions.
Q: Is there any chance a prod-level asset will be provided at some point, perhaps adding more i18n support et al.?
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.
We are not expecting expanded character support 🙁
I will check and see if there is a more recent version.
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.
Updated with the final* versions.
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.
I missed this one:
removed in protocol-tokens#116
(I'm preparing a tokens.config doc update and a changelog entry for tokens version bump so I'll include this fix with it…)