-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
style(theme): refine font-family #1162
Conversation
'Source Han Sans CJK TC', 'Hiragino Sans', 'Noto Sans CJK JP', | ||
'Source Han Sans CJK JA', 'Apple SD Gothic Neo', 'Noto Sans CJK KR', | ||
'Source Han Sans CJK KO', sans-serif; | ||
--vp-font-family-mono: 'SF Mono', 'Source Code Pro', 'Fira Code', Menlo, |
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.
SF Mono is a system font, so adding it to the list and adding it before Menlo (another system font for the same platform) if it's preferred: that makes sense.
I wouldn't add non-system fonts that the theme is not loading, such as Source Code Pro and Fira Code. This seems unnecessary with good enough coverage for system fonts, and when ending the list with the generic family name (monospace
). It can also lead to even more rendering differences between users, where not just the platform but also what fonts and what version of fonts users may have installed will play a part.
(Side note: quoting font names is unnecessary. Both 'SF Mono'
and SF Mono
should work.)
'Source Han Sans CJK JA', 'Apple SD Gothic Neo', 'Noto Sans CJK KR', | ||
'Source Han Sans CJK KO', sans-serif; | ||
--vp-font-family-mono: 'SF Mono', 'Source Code Pro', 'Fira Code', Menlo, | ||
Monaco, Consolas, 'Courier New', monospace; |
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.
Personally I would remove Courier New from this list.
If the goal is to show whatever monospace font the OS or browser has configured, using generic names like --vp-font-family-mono: monospace, monospace;
(need to have a name before the generic monospace
at the end because of old browser text sizing issues with font-family: monospace
) is enough.
I read the existing value as an attempt to select a more specific style of monospace font, one with big x-height and no serifs, versus Courier New which has a smallish x-height and slab serifs (and suffers from readability issues at small or medium sizes as a result).
So if none of the listed readable monospace fonts doesn't match, it's probably better to fall back to monospace
and let the browser decide (which may result in Courier New or something else, e.g. it's definitely not Courier New on Android), rather than explicitly ask for Courier New.
@@ -147,8 +147,14 @@ | |||
:root { | |||
--vp-font-family-base: 'Inter var experimental', 'Inter var', -apple-system, | |||
BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen, Ubuntu, Cantarell, | |||
'Fira Sans', 'Droid Sans', 'Helvetica Neue', sans-serif; | |||
--vp-font-family-mono: Menlo, Monaco, Consolas, 'Courier New', monospace; | |||
'Fira Sans', 'Droid Sans', 'Helvetica Neue', 'Helvetica', 'Noto Sans', |
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 wonder if it's worth listing so many sans-serif fonts here.
Is listing 13 CJK fonts an improvement over shorter options, such as:
- Just defaulting to
sans-serif
, so:--vp-font-family-base: Inter var experimental, Inter var, sans-serif;
- Or defaulting to
system-ui
first, then the genericsans-serif
:--vp-font-family-base: Inter var experimental, Inter var, system-ui, sans-serif;
If there is a need to specify more fonts because system and browser default fonts for CJK scripts tend to look bad, would there be a way to limit it to just a few options and not ~13 maybe?
Notes:
- Browser support for
system-ui
is good: https://caniuse.com/font-family-system-ui -apple-system
andBlinkMacSystemFont
are not needed anymore, usesystem-ui
instead.
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 usage of the CJK fonts here is indeed a bit strange. There are several possible problems here:
-
If the web page (or a part of the web page, because a web page can be multilingual) is not properly language tagged, it may cause the wrong font to be applied (like this example).
-
IIRC the Noto CJK fonts have been renamed, but here it only contains the old names.
-
There might be potential issues with
system-ui
. See [css-fonts]system-ui
behavior is undesired for some users w3c/csswg-drafts#3658
(See also w3c/clreq#408 (comment) for related discussions about Chinese fonts.)
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.
To avoid this kind of issue, maybe we should just list the Latin fonts here, not the fonts for other scripts.
Noted that
SF Mono
is much better thanMenlo
on Apple devices.