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

Font Face: avoid using non required property as index and use right property to render font face #54610

Closed
wants to merge 3 commits into from

Conversation

matiasbenedetto
Copy link
Contributor

What?

  • Fixes font face rendering when a name property is not defined.
  • Fix the potential error when rendering font face CSS font family.

Why?

Breaking with fonts families definitions, not including a name property.

How?

  • Avoid using non-required property as the index of an associative array.
  • Use the right property to render font face. (fontFamily property of a font face).

Testing Instructions

  • Activate a theme with font families not containing the name property set.

Todo:

Fix the tests accordingly to these changes.

@github-actions
Copy link

github-actions bot commented Sep 19, 2023

Warning: Type of PR label error

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Accessibility (a11y), [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: [Feature] Typography.

Read more about Type labels in Gutenberg.

@github-actions
Copy link

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/compat/wordpress-6.4/fonts/font-face/class-wp-font-face-resolver.php

@matiasbenedetto matiasbenedetto added the [Feature] Typography Font and typography-related issues and PRs label Sep 19, 2023
@github-actions
Copy link

Flaky tests detected in 558b2ad.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6237676250
📝 Reported issues:

// Skip if font-family "name" is not defined.
if ( empty( $definition['name'] ) ) {
// Skip if font-family "slug" is not defined.
if ( empty( $definition['slug'] ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it should use slug. Instead I think it should use fontFamily. Why? slug is not the proper name of the Font Family, but rather is the lowercase and hyphen version of the name. Currently Font Face uses this font-family name in the CSS.

I have PR coming shortly to show it in action.

Copy link
Contributor

Choose a reason for hiding this comment

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

PR #54615 uses the fontFamily setting as the fallback to get the name.

@matiasbenedetto
Copy link
Contributor Author

Sorry for the overlap. Closing in favor of #54610.

@bph
Copy link
Contributor

bph commented Oct 4, 2023

PR #54615 fixes the issue #54605

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Typography Font and typography-related issues and PRs
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

'name' attribute is now required on Font Face definitions (breaking many existing themes)
3 participants