Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Fix font paths #51

Merged
merged 2 commits into from
Oct 8, 2021
Merged

Fix font paths #51

merged 2 commits into from
Oct 8, 2021

Conversation

aristath
Copy link
Member

@aristath aristath commented Oct 7, 2021

We can't assume that the theme will be in wp-content/themes/twentytwentytwo. Some sites use a different folder for wp-content, they have themes in subfolders, they might rename the theme and so on.
As a result, using URLs like url('/wp-content/themes/twentytwentytwo/assets/fonts/source-serif-pro/SourceSerif4Variable-Roman.ttf.woff2') in the stylesheet has a potential to break things.

A solution to that issue would be to add the font-face styles inline. This will allow us to use PHP to get the correct URL to the theme, and avoid breaking things.

Notes:

  • This PR changes twentytwentytwo-style from get_template_directory_uri() . '/style.css' to an empty string (''). Since the stylesheet is completely empty and only contains the headers for the theme, loading it would be a waste of resources. Using an empty string allows us to inline styles using wp_add_inline_style, without actually enqueueing a file.
  • This PR also removes non-woff2 files. Since we no longer support IE11, these are not needed (also see Remove .otf & .woff font-files #50)

@jffng
Copy link
Collaborator

jffng commented Oct 7, 2021

Thanks @aristath, curious what others think.

Also I'm hoping we will be able to leverage https://core.trac.wordpress.org/ticket/46370#comment:53 so we can declare fonts via theme.json.

@jffng jffng added this to In progress in Twenty Twenty-Two Project Board via automation Oct 7, 2021
@jffng jffng moved this from In progress to Needs Review in Twenty Twenty-Two Project Board Oct 7, 2021
@kjellr
Copy link
Collaborator

kjellr commented Oct 7, 2021

Interesting approach! I think this works for now, though we'll need to remember to re-enqueue the actual style.css file if we end up needing to pull in any CSS later on. (I'm keeping my fingers crossed that we won't!)

functions.php Outdated
Comment on lines 28 to 36
src: url('" . get_theme_file_uri( 'assets/fonts/source-serif-pro/SourceSerif4Variable-Roman.ttf.woff2' ) . "') format('woff2');
}

@font-face{
font-family: 'Source Serif Pro';
font-weight: 200 900;
font-style: italic;
font-stretch: normal;
src: url('" . get_theme_file_uri( 'assets/fonts/source-serif-pro/SourceSerif4Variable-Italic.ttf.woff2' ) . "') format('woff2');
Copy link
Member

Choose a reason for hiding this comment

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

Why not just have the URL start with an assets/ relative path? See #57

Copy link
Member Author

@aristath aristath Oct 8, 2021

Choose a reason for hiding this comment

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

Small CSS files can be inlined (see #49 for example). If the style.css file gets inlined, relative paths are no longer relative to the style.css path but to the site's root - and in that case assets/ won't work.

@aristath
Copy link
Member Author

aristath commented Oct 8, 2021

Rebased the branch and there are no more conflicts 👍

Copy link
Collaborator

@jffng jffng left a comment

Choose a reason for hiding this comment

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

This fixes font #32 also. Let's go with this for now, thanks @aristath.

@jffng jffng merged commit 674339f into WordPress:trunk Oct 8, 2021
Twenty Twenty-Two Project Board automation moved this from Needs Review to Done Oct 8, 2021
@kjellr kjellr mentioned this pull request Oct 14, 2021
jeffpaul added a commit to jeffpaul/twentytwentytwo that referenced this pull request Nov 16, 2021
Note that some of the references below are not the only contributions (commits, PR review, helpful issue input, etc.) from these folks, but merely the first ones I encountered while reviewing items committed to `trunk` since the initial branch commit.

@westonruter via  WordPress#51
@ntwb via WordPress#73
@juricav via WordPress#113
@Sandstromer via WordPress#69
@jasmussen via WordPress#74
@melchoyce via WordPress#16
@Riyadh1734 via WordPress#182
@desrosj via WordPress#223
@beafialho via WordPress#172
@clucasrowlands via WordPress#171
@Otto42 via WordPress#28
@luminuu via WordPress#107
@felixarntz via WordPress#240
kjellr added a commit that referenced this pull request Nov 17, 2021
* add missing props to CONTRIBUTORS.md

Note that some of the references below are not the only contributions (commits, PR review, helpful issue input, etc.) from these folks, but merely the first ones I encountered while reviewing items committed to `trunk` since the initial branch commit.

@westonruter via  #51
@ntwb via #73
@juricav via #113
@Sandstromer via #69
@jasmussen via #74
@melchoyce via #16
@Riyadh1734 via #182
@desrosj via #223
@beafialho via #172
@clucasrowlands via #171
@Otto42 via #28
@luminuu via #107
@felixarntz via #240

* add dotorg handles to CONTRIBUTORS.md

* Fix Rich's WP.org username.

Co-authored-by: Kjell Reigstad <kjell@kjellr.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants