-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
feat: more advanced font specification #1501
base: v4
Are you sure you want to change the base?
Conversation
will run prettier again in a second, sorry ^_^ |
interface FontInfo { | ||
/** | ||
* e.g. "ital,wght@0,400;1,200" | ||
*/ | ||
features: string | ||
} |
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.
If you're going to separate the fonts from the features, why not use multiple values for the different options? Like instead of just features
, maybe consider mode
for italics/bold/default and weight
for the font weight.
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 is reasonable and I thought about it, but
- if Google Fonts changes their API, this code would need to be updated quickly,
- making it as expressive as the string while keeping a good API would be hard,
- specifying the particular weights is something that only people who are already probably heavily editing the theme are going to think about, and those people are probably already familiar with the Google Fonts API
so I decided that keeping it a string would be less of a maintenance burden and less complex in most situations.
(If it seems like most people would prefer this be broken out into multiple options anyways, I can update it to do that / give people both options with features
winning out.)
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 is reasonable and I thought about it, but
- if Google Fonts changes their API, this code would need to be updated quickly,
- making it as expressive as the string while keeping a good API would be hard,
- specifying the particular weights is something that only people who are already probably heavily editing the theme are going to think about, and those people are probably already familiar with the Google Fonts API
so I decided that keeping it a string would be less of a maintenance burden and less complex in most situations.(If it seems like most people would prefer this be broken out into multiple options anyways, I can update it to do that / give people both options with
features
winning out.)
I feel like user that want to heavily customize their fonts would do so in custom.scss
anyway.
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.
My issue with that is that the current method of specifying fonts outputs a to Google Fonts at the top - I want to be able to modify that in a more advanced way. While I could just put an @import
in my custom.scss, if the Google Fonts element is incorrect, that's another request to a resource that might not actually be used / be what I want.
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.
My issue with that is that the current method of specifying fonts outputs a to Google Fonts at the top - I want to be able to modify that in a more advanced way. While I could just put an
@import
in my custom.scss, if the Google Fonts element is incorrect, that's another request to a resource that might not actually be used / be what I want.
In that case, you could just change the font source in your quartz.config.ts
to local and set the desired font in your custom.scss
through @font-face
, be it a Google font or not.
I suppose I'm kinda looking for what problem is solved with the proposed change. The current configuration makes it easy for inexperienced users to configure a Google-font. Advanced users would likely use CSS. I'm unsure what group is better served by the proposed changes.
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 think for cases like custom fonts, users best to use local
and setup @font-face
in custom.scss
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.
no need to add unnecessary abstraction on top
This adds the ability to specify multiple fonts (e.g. for fallback purposes), as well as specifying font features for Google Fonts. The only related issue I could find was #609, which appears to have been closed without any progress?
A resultant config might look like:
Fonts that do not have features sent in the
fonts
object inherit the default features for header/body/code fonts that existed before this PR, so this is strictly a backwards-compatible change.