-
Notifications
You must be signed in to change notification settings - Fork 10
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
Preload fonts in use as per CSS definitions #765
Conversation
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.
Small nitpicks, other than that lgtm.
|
||
public static partial class FontPreloader | ||
{ | ||
private static List<string>? FontUriCache = null!; |
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.
readonly field or getter only property.
Also IReadOnlyCollection instead of List.
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.
Does this need to be thread safe?
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.
Ah. Nevermind. If I understood correctly, the whole logic is only executed once.
private static List<string>? FontUriCache = null!; | ||
|
||
public static async Task<IEnumerable<string>> GetFontUrisAsync() => FontUriCache ??= await LoadFontUrisAsync(); | ||
public static async Task<List<string>> LoadFontUrisAsync() |
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.
Can be made private
return FontUriCache; | ||
} | ||
|
||
[GeneratedRegex(@"url\([""']?([^""'\)]+)[""']?\)", RegexOptions.Multiline | RegexOptions.Compiled)] |
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.
Maybe stricter including src:
key?
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.
What do you think about adding also the file extension here.
the url function could in theory load any kind of file.
foreach (Match match in matches) | ||
{ | ||
if (match.Success) | ||
FontUriCache.Add($"/_static/{match.Groups[1].Value}"); |
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.
In order to preload fonts, a hint must be given to the browser in the form of a tag:
<link rel="preload" href="/path/to/font.woff2" as="font" type="font/woff2" crossorigin>
This PR includes a cached repository for the font definitions in .css files so we don't need to worry about adding these if fonts change.
Handles #704