Make WordPress Core

Opened 4 years ago

Closed 3 months ago

Last modified 3 months ago

#48630 closed enhancement (fixed)

Twenty Twenty: please use wp_enqueue_style for the fonts

Reported by: markhowellsmead's profile markhowellsmead Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.5 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: 2nd-opinion dev-feedback has-patch needs-testing
Focuses: ui, performance Cc:

Description

Please please use wp_enqueue_style for the fonts (not CSS @import), so that they can be dequeued by a Child Theme or Plugin. (In case the Child Theme or Plugin wants to use different fonts.)

Attachments (2)

48630.diff (28.9 KB) - added by poena 5 months ago.
Move the font-face CSS to a separate file and enqueue the file.
48630-2.diff (19.2 KB) - added by poena 3 months ago.
Move the Inter font-face declaration to a separate file

Download all attachments as: .zip

Change History (19)

#1 follow-up: @nielslange
4 years ago

@markhowellsmead Where do you see @import statements for fonts?

#2 in reply to: ↑ 1 ; follow-up: @markhowellsmead
4 years ago

Replying to nielslange:

@markhowellsmead Where do you see @import statements for fonts?

Sorry, my bad - they're directly-referenced srcs in @font-face statements.

https://github.com/WordPress/twentytwenty/blob/master/style.css#L254

#3 in reply to: ↑ 2 @nielslange
4 years ago

  • Keywords needs-patch added

Sorry, my bad - they're directly-referenced srcs in @font-face statements.

https://github.com/WordPress/twentytwenty/blob/master/style.css#L254

Don't worry and thanks for clarifying this! 😉 I see what you mean now and will create a patch for that. If the patch gets accepted is another story though. 😁

#5 follow-up: @Otto42
4 years ago

I'm confused as to what the issue here is. If a child theme wants to use different fonts, it can do so simply by defining them as such in the child theme CSS.

#6 in reply to: ↑ 5 @markhowellsmead
4 years ago

Replying to Otto42:

I'm confused as to what the issue here is. If a child theme wants to use different fonts, it can do so simply by defining them as such in the child theme CSS.

This true, but the font files from the parent theme will still be loaded, which is a(n albiet minor) performance hit.

#7 @ianbelanger
4 years ago

  • Keywords 2nd-opinion dev-feedback added; needs-patch removed

#8 @poena
4 years ago

I agree that non system fonts should be enqueued.
Though I should probably learn more about variable fonts before I open my mouth ;)

Last edited 4 years ago by poena (previous) (diff)

#9 @ianbelanger
4 years ago

#48922 was marked as a duplicate.

This ticket was mentioned in Slack in #themereview by poena. View the logs.


4 years ago

@poena
5 months ago

Move the font-face CSS to a separate file and enqueue the file.

#11 @poena
5 months ago

  • Keywords has-patch needs-testing added
  • Milestone changed from Awaiting Review to Future Release

Testing instructions

Apply patch 48630.diff.

Block editor & front of the website:
Confirm that the new CSS file, assets/css/font.css is loading. You can use your browser's developer tools to check this.
Confirm that the post title uses the correct font family (Inter) and looks the same with and without the patch.

Classic editor:
Install and activate the classic editor plugin.
https://wordpress.org/plugins/classic-editor/
(Alternatively test on an older WordPress version that does not have the block editor)

Confirm that the new CSS file, assets/css/font.css is loading. You can use your browser's developer tools to check this.

Add an H1 heading to the the classic editor, and confirm that it uses the correct font family (Inter) and that it looks the same with and without the patch.

Next please change the WordPress installation to an RTL language and confirm that font family is correct in the editor and on the front.

#12 @SergeyBiryukov
3 months ago

  • Milestone changed from Future Release to 6.5
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


3 months ago

#14 @mukesh27
3 months ago

  • Keywords needs-refresh added

This ticket was discussed during the performance bug scrub.

@poena the 48630.diff did not have font.css file. Could you please add it? Or open new PR.

Additional props: @joemcgill

@poena
3 months ago

Move the Inter font-face declaration to a separate file

#15 @poena
3 months ago

  • Keywords needs-refresh removed

@mukesh27 Thank you for notifying me, I had forgotten to use
svn add. Luckily I still had the files.

I made two other changes:
Updated the since.
Reverted the part where I had moved the CSS for the NonBreakingSpaceOverride.

#16 @SergeyBiryukov
3 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 57311:

Twenty Twenty: Move the Inter font declaration to a separate file and enqueue the file.

This allows the font to be dequeued by a child theme or plugin.

Props poena, markhowellsmead, nielslange, Otto42, SGr33n, mukesh27, joemcgill.
Fixes #48630.

#17 @SergeyBiryukov
3 months ago

In 57313:

Twenty Twenty: Add missing comma in twentytwenty_classic_editor_styles().

This resolves a WPCS error:

There should be a comma after the last array item in a multi-line array.

Follow-up to [57311].

See #48630.

Note: See TracTickets for help on using tickets.