Skip to content

Drop the usage of the local CSS rule#3826

Closed
Luehrsen wants to merge 9 commits intoWordPress:trunkfrom
Luehrsen:update/drop-local-webfonts_handler
Closed

Drop the usage of the local CSS rule#3826
Luehrsen wants to merge 9 commits intoWordPress:trunkfrom
Luehrsen:update/drop-local-webfonts_handler

Conversation

@Luehrsen
Copy link
Copy Markdown

@Luehrsen Luehrsen commented Jan 7, 2023

As discussed, loading the local font file can lean to unexpected compatibility issues due to version or locale mismatches of the font.

Vendors like Google have dropped using local in their font delivery.

That is why I would propose dropping the local rule altogether.

Trac ticket: https://core.trac.wordpress.org/ticket/57430


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link
Copy Markdown
Contributor

@costdev costdev 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 the PR @Luehrsen! LGTM 👍

Comment on lines +3510 to +3515
$items[] = ( 'data' === $item['format'] )
? "url({$item['url']})"
: "url('{$item['url']}') format('{$item['format']}')";
}

return $src;
return implode( ', ', $items );
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change is out-of-scope for the defect bugfix. Let's instead keep parity with the fix made in Gutenberg WordPress/gutenberg#47254.

I'll update this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Luehrsen Though I do understand why this change was made, I've reverted it for the commit.

Why? To keep it matching (parity with) the same code in the Fonts API (being developed in Gutenberg).

This is intentional to keep what's in Core synchronized with what's in the API being developed in Gutenberg.

This change could be considered in the Fonts API. Please feel free to open an issue and PR for discussion. Thank you!

Copy link
Copy Markdown
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

  • Confirmed changes match (have parity with) the same fix made in the Font API WordPress/gutenberg#47254
  • Has change annotations ✅
  • Works as expected ✅

Ready for commit 👍

@hellofromtonya
Copy link
Copy Markdown
Contributor

Committed via https://core.trac.wordpress.org/changeset/55314. Thanks for your contributions!

@Luehrsen Luehrsen deleted the update/drop-local-webfonts_handler branch February 13, 2023 14:36
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.

4 participants