-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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 a setting allowing the use of system's default font in Web UI #4033
Conversation
I've been wanting to look into this for a while, glad someone beat me to it 👍 A couple of questions:
|
|
I'm confused about this implementation. This doesn't work at all if I've changed my system's default font, which seems to kinda defeat the point. Why not just use font-family: sans-serif; and be done with it? or font-family: none? |
let className = 'ui'; | ||
|
||
if (this.props.systemFontUi) | ||
className += ' has-system-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.
this would be more cleanly implemented as an array, using classes.join(" ") at the calling site (or maybe react has a better pattern for this?)
@nightpool I think Chrome's fonts can be changed in its settings (don't know if it reads them from the OS everytime you launch it or just when you start it the first time). |
@nightpool You'll notice that we're talking about the system's default font. Quoting the specification for
So if your browser implements |
@sorin-davidoi right. But using "Segoe UI" means that, if I was on Windows 10 (or another OS where I happened to have installed Segoe UI for unrelated reasons....), that would override my chrome font choice. If we want to just use system-ui, then fine. But having the other fonts hard coded seems like a bad decision. |
@nightpool I agree, it is messy. Github seems to use something similar to this (https://css-tricks.com/os-specific-fonts-css/#article-header-id-0). I would personally be happy with |
The other fonts are merely fallbacks for all the other browsers that don't support
Keep in mind that whatever font that you choose in the setting of your browser is the fallback used for Also, the setting is disabled by default, and usually people who would want to enable it know what they're getting into. |
app/javascript/styles/basics.scss
Outdated
@@ -61,3 +61,7 @@ button { | |||
align-items: center; | |||
justify-content: center; | |||
} | |||
|
|||
.has-system-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.
Minor nitpick, maybe something like .system-font
would be better? has
for me implies that we do some feature detection (e.g. has-flex
, has-grid
).
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 would name it '.use-system-font'
@sorin-davidoi I tested and Roboto is not loaded (except for the regular family because of the style on |
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.
Off by default, saves ~300 KB if enabled 👍
return ( | ||
<div className='ui' ref={this.setRef}> | ||
<div className={classNames.join(' ')} ref={this.setRef}> |
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.
How about using classnames
? It's used in some components already.
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.
@unarist I didn't know about that! I'll update the code to use it for consistency.
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.
@unarist I just pushed a commit to use classnames
! 👍
app/javascript/styles/basics.scss
Outdated
// Droid Sans => Older Androids (<4.0) | ||
// Helvetica Neue => Older macOS <10.11 | ||
// Roboto => web-font fallback and newer Androids (>=4.0) | ||
font-family: system-ui, -apple-system,BlinkMacSystemFont, "Segoe UI","Oxygen", "Ubuntu","Cantarell","Fira Sans","Droid Sans","Helvetica Neue","Roboto", sans-serif; |
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.
hey, the webfont for Roboto is actually called "mastodon-font-sans-serif" in our CSS
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.
@Gargron I just fixed this. Sorry for the overlooking 😅
…stodon#4033) * add a system_font_ui setting on the server * Plug the system_font_ui on the front-end * add EN/FR locales for the new setting * put Roboto after all other fonts * remove trailing whitespace so CodeClimate is happy * fix user_spec.rb * correctly write user_spect this time * slightly better way of adding the classes * add comments to the system-font stack for clarification * use .system-font for the class instead * don't use multiple lines for comments * remove trailing whitespace * use the classnames module for consistency * use `mastodon-font-sans-serif` instead of Roboto directly
…stodon#4033) * add a system_font_ui setting on the server * Plug the system_font_ui on the front-end * add EN/FR locales for the new setting * put Roboto after all other fonts * remove trailing whitespace so CodeClimate is happy * fix user_spec.rb * correctly write user_spect this time * slightly better way of adding the classes * add comments to the system-font stack for clarification * use .system-font for the class instead * don't use multiple lines for comments * remove trailing whitespace * use the classnames module for consistency * use `mastodon-font-sans-serif` instead of Roboto directly
Disabled by default
This will use the following font stack: