Skip to content
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

use font-display optional instead of swap #5334

Closed
wants to merge 1 commit into from
Closed

Conversation

patrickkettner
Copy link
Collaborator

fixes #5332

@sebastianbenz
Copy link
Collaborator

Did you check whether layouts break with the default font?

@sebastianbenz
Copy link
Collaborator

We should also make sure to match the fallback font to the current font as much as possible.

@patrickkettner
Copy link
Collaborator Author

Did you check whether layouts break with the default font?

None of the pages I looked at have breaking changes. Would it be worth doing something like Percy for this?

We should also make sure to match the fallback font to the current font as much as possible.

do you mean match the spacing/weight or the declared font (or both)? I am not sure how much tweaking you are interested in having done

Separately, we do not seem to be preloading or prefetching the fonts on the at any place in the repo, from what I can grep. Should that be added?

@sebastianbenz
Copy link
Collaborator

Percy seems like overkill. It'd just be nice to have the fonts roughly matched. I've created #5335 to fallback to the system font instead. That works good enough in my opinion.

You're right, we're not preloading the fonts. I'd sworn that I've added this back in the day. I'm not sure whether we should add it or not. It might block bandwidth on poor connections.

@patrickkettner
Copy link
Collaborator Author

patrickkettner commented Feb 9, 2021

It might block bandwidth on poor connections.

I didn't think that a font prefetch that is optional would block, but that makes sense

sebastianbenz added a commit that referenced this pull request Feb 10, 2021
* use font-display optional instead of swap

* add System font as Poppins and Noto Sans fallback

Co-authored-by: Patrick Kettner <patrickkettner@gmail.com>
@sebastianbenz
Copy link
Collaborator

Is prefetch needed in case of font-display optional? I thought the browser will download the font anyway.

@patrickkettner
Copy link
Collaborator Author

sorry, no, thats not what I meant. Rather, I meant if you prefetch an optional font, I did not think it would ever block a connection.

@matthiasrohmer
Copy link
Collaborator

Created #5336 to track font matching.

@sebastianbenz, no worries, your memory is working fine. We've had preload statements for the fonts and removed them midst September which according to Kevin had a positive effect: #4533 (comment).

@patrickkettner
Copy link
Collaborator Author

patrickkettner commented Feb 10, 2021 via email

@patrickkettner patrickkettner deleted the font-swap branch February 16, 2021 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update font-display from swap to optional
3 participants