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

Archeo: Add fonts to theme.json #5609

Merged
merged 6 commits into from
Mar 17, 2022
Merged

Archeo: Add fonts to theme.json #5609

merged 6 commits into from
Mar 17, 2022

Conversation

scruffian
Copy link
Member

@scruffian scruffian commented Mar 2, 2022

Do not merge until GB 12.8 is on wpcom

Changes proposed in this Pull Request:

This moves the font definitions to the theme.json file. This was made possible because WordPress/gutenberg#37140 was merged.

To test check that the font loads as before.

I'm not sure if we can also remove the pre-loading script.

Related issue(s):

WordPress/gutenberg#37140

@scruffian scruffian added the [Theme] Archeo Automatically generated label for Archeo. label Mar 2, 2022
@scruffian scruffian requested a review from a team March 2, 2022 10:57
@scruffian scruffian self-assigned this Mar 2, 2022
@mikachan mikachan added this to the Archeo milestone Mar 7, 2022
Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! This seems to work great for me, all the fonts load as they did before. The only difference I could see was that the main font files are no longer preloaded, so I think we should keep that function as you mentioned.

@MaggieCabrera
Copy link
Contributor

We should hold off on merging this until it makes it to dotcom, right?

@mikachan
Copy link
Member

mikachan commented Mar 7, 2022

Yes I think so, it looks like it'll be included in GB 12.8.

@MaggieCabrera
Copy link
Contributor

We should do this to Stewart, Livro and Remote too

@scruffian
Copy link
Member Author

We should do this to Stewart, Livro and Remote too

Done

@scruffian
Copy link
Member Author

Do you think we should be preloading fonts? This comment is very interesting. Maybe we just need to change font-display to 'block'? See https://css-tricks.com/almanac/properties/f/font-display/

@MaggieCabrera
Copy link
Contributor

Do you think we should be preloading fonts? This comment is very interesting. Maybe we just need to change font-display to 'block'? See https://css-tricks.com/almanac/properties/f/font-display/

Makes sense to me, if CSS already has that option

@scruffian
Copy link
Member Author

I added a commit to remove the code that preloads fonts. Instead we can use font-display: block which means that if the font is available we'll use it but if not then we'll fallback to a default so that users can at least read the content.

@MaggieCabrera
Copy link
Contributor

I think we can bring this in now. I tested on my sandbox and it's working, someone else have a look and then we can merge, please?

@jffng
Copy link
Contributor

jffng commented Mar 17, 2022

I think we can bring this in now. I tested on my sandbox and it's working, someone else have a look and then we can merge, please?

Tested across all affected themes and the fonts are loading as expected.

@jffng jffng merged commit 38d6bc4 into trunk Mar 17, 2022
@jffng jffng deleted the update/archeo-web-fonts branch March 17, 2022 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Theme] Archeo Automatically generated label for Archeo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants