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.

Sign up for GitHub

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

Google Fonts: enqueue only picked font families in Gutenberg #23810

Closed
wants to merge 8 commits into from
Closed

Google Fonts: enqueue only picked font families in Gutenberg #23810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
30563a2
Move Google Fonts module to its own folder
zaguiini Apr 5, 2022
0f4668d
Convert the module into a class so we can share state between hooks
zaguiini Apr 5, 2022
279c275
Instropect blocks and enqueue used Google fonts
zaguiini Apr 5, 2022
8b31d4a
Instropect global styles and enqueue used Google fonts
zaguiini Apr 5, 2022
2896b93
Get font family slug through wp_register_webfonts output
zaguiini Apr 6, 2022
9bab79d
Extract global styles webfonts introspector to its own package
zaguiini Apr 6, 2022
b660425
Use webfonts introspector when looking for fonts used in global styles
zaguiini Apr 6, 2022
b9c92a4
Add CHANGELOG entry
zaguiini Apr 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 16 additions & 0 deletions projects/packages/global-styles-webfonts-introspector/.gitattributes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# Files not needed to be distributed in the package.
.gitattributes export-ignore
.github/ export-ignore
package.json export-ignore

# Files to include in the mirror repo, but excluded via gitignore
# Remember to end all directories with `/**` to properly tag every file.
# /src/js/example.min.js production-include

# Files to exclude from the mirror repo, but included in the monorepo.
# Remember to end all directories with `/**` to properly tag every file.
.gitignore production-exclude
changelog/** production-exclude
phpunit.xml.dist production-exclude
.phpcs.dir.xml production-exclude
tests/** production-exclude
2 changes: 2 additions & 0 deletions projects/packages/global-styles-webfonts-introspector/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
vendor/
node_modules/
24 changes: 24 additions & 0 deletions projects/packages/global-styles-webfonts-introspector/.phpcs.dir.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?xml version="1.0"?>
<ruleset>

<rule ref="WordPress.WP.I18n">
<properties>
<property name="text_domain" type="array">
<element value="jetpack-global-styles-webfonts-introspector" />
</property>
</properties>
</rule>
<rule ref="Jetpack.Functions.I18n">
<properties>
<property name="text_domain" value="jetpack-global-styles-webfonts-introspector" />
</properties>
</rule>

<rule ref="WordPress.Utils.I18nTextDomainFixer">
<properties>
<property name="old_text_domain" type="array" />
<property name="new_text_domain" value="jetpack-global-styles-webfonts-introspector" />
</properties>
</rule>

</ruleset>
7 changes: 7 additions & 0 deletions projects/packages/global-styles-webfonts-introspector/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Changelog

All notable changes to this project will be documented in this file.

The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

20 changes: 20 additions & 0 deletions projects/packages/global-styles-webfonts-introspector/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# global-styles-webfonts-introspector

Looks for webfonts picked in global styles

## How to install global-styles-webfonts-introspector

### Installation From Git Repo

## Contribute

## Get Help

## Security

Need to report a security vulnerability? Go to [https://automattic.com/security/](https://automattic.com/security/) or directly to our security bug bounty site [https://hackerone.com/automattic](https://hackerone.com/automattic).

## License

global-styles-webfonts-introspector is licensed under [GNU General Public License v2 (or later)](./LICENSE.txt)

Empty file added projects/packages/global-styles-webfonts-introspector/changelog/.gitkeep
View file
Open in desktop
Empty file.
4 changes: 4 additions & 0 deletions ...nts-introspector/changelog/update-instrospect-google-fonts-instead-of-enqueueing-them-all
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: added

Add Google fonts global styles introspection package.
44 changes: 44 additions & 0 deletions projects/packages/global-styles-webfonts-introspector/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
{
"name": "automattic/jetpack-global-styles-webfonts-introspector",
"description": "Looks for webfonts picked in global styles",
"type": "jetpack-library",
"license": "GPL-2.0-or-later",
"require": {},
"require-dev": {
"yoast/phpunit-polyfills": "1.0.3",
"automattic/jetpack-changelogger": "^3.0"
},
"autoload": {
"classmap": [
"src/"
]
},
"scripts": {
"phpunit": [
"./vendor/phpunit/phpunit/phpunit --colors=always"
],
"test-coverage": [
"php -dpcov.directory=. ./vendor/bin/phpunit --coverage-clover \"$COVERAGE_DIR/clover.xml\""
],
"test-php": [
"@composer phpunit"
]
},
"repositories": [
{
"type": "path",
"url": "../../packages/*",
"options": {
"monorepo": true
}
}
],
"minimum-stability": "dev",
"prefer-stable": true,
"extra": {
"branch-alias": {
"dev-master": "0.1.x-dev"
},
"textdomain": "jetpack-global-styles-webfonts-introspector"
}
}
29 changes: 29 additions & 0 deletions projects/packages/global-styles-webfonts-introspector/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
{
"private": true,
"name": "@automattic/jetpack-global-styles-webfonts-introspector",
"version": "0.1.0-alpha",
"description": "Looks for webfonts picked in global styles",
"homepage": "https://jetpack.com",
"bugs": {
"url": "https://github.com/Automattic/jetpack/issues"
},
"repository": {
"type": "git",
"url": "https://github.com/Automattic/jetpack.git"
},
"license": "GPL-2.0-or-later",
"author": "Automattic",
"scripts": {
"build": "echo 'Not implemented.'",
"build-js": "echo 'Not implemented.'",
"build-production": "echo 'Not implemented.'",
"build-production-js": "echo 'Not implemented.'",
"clean": "true"
},
"devDependencies": {},
"engines": {
"node": "^14.18.3 || ^16.13.2",
"pnpm": "^6.32.3",
"yarn": "use pnpm instead - see docs/yarn-upgrade.md"
}
}
14 changes: 14 additions & 0 deletions projects/packages/global-styles-webfonts-introspector/phpunit.xml.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<phpunit bootstrap="tests/php/bootstrap.php" backupGlobals="false" colors="true" convertDeprecationsToExceptions="true">
<testsuites>
<testsuite name="main">
<directory prefix="test" suffix=".php">tests/php</directory>
</testsuite>
</testsuites>
<filter>
<whitelist processUncoveredFilesFromWhitelist="false">
<!-- Better to only include "src" than to add "." and then exclude "tests", "vendor", and so on, as PHPUnit still scans the excluded directories. -->
<!-- Add additional lines for any files or directories outside of src/ that need coverage. -->
<directory suffix=".php">src</directory>
</whitelist>
</filter>
</phpunit>
90 changes: 90 additions & 0 deletions ...ges/global-styles-webfonts-introspector/src/class-global-styles-webfonts-introspector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
<?php
jeyip marked this conversation as resolved.
Show resolved Hide resolved
/**
* Global styles webfonts introspector
*
* @package automattic/global-styles-webfonts-introspector
* @since 0.1.0
*/

namespace Automattic\Jetpack\Fonts;

/**
* Global styles webfonts introspector.
*/
class Global_Styles_Webfonts_Introspector {
/**
* Extract the font family slug from a settings object.
*
* @param object $setting The setting object.
*
* @return string|void
*/
private static function extract_font_slug_from_setting( $setting ) {
if ( isset( $setting['typography'] ) && isset( $setting['typography']['fontFamily'] ) ) {
$font_family = $setting['typography']['fontFamily'];

// Full string: var(--wp--preset--font-family--slug).
// We do not care about the origin of the font, only its slug.
preg_match( '/font-family--(?P<slug>.+)\)$/', $font_family, $matches );

if ( isset( $matches['slug'] ) ) {
return $matches['slug'];
}

// Full string: var:preset|font-family|slug
// We do not care about the origin of the font, only its slug.
preg_match( '/font-family\|(?P<slug>.+)$/', $font_family, $matches );

if ( isset( $matches['slug'] ) ) {
return $matches['slug'];
}

return $font_family;
Copy link
Contributor

@jeyip jeyip Apr 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Could you describe what this line would return and under what circumstances this condition would be triggered? I'm guessing one of the presets we evaluate ( blocks, HTML, or global typography ) return a normal slug.

Also completely unrelated to this PR, but it seems so odd to have so many different ways of representing the font family slug 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also completely unrelated to this PR, but it seems so odd to have so many different ways of representing the font family slug 🙃

Right! That's because you can specify a CSS variable to link it inside theme.json. You can also pick a font in the editor, and then it would become the second format var:preset|font-family|slug, which is the one that's registered in the global styles CPT.

But then, you can also solely specify the slug, and that's why we're returning... If none of the formats match, just return what's in there and we'll try our luck. It might be a system font, might be just the slug... Who knows.

}
}

/**
* Introspect global styles for webfonts.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add an @return inline comment here for consistency?

*/
public static function introspect_global_styles_webfonts() {
if ( ! function_exists( 'gutenberg_get_global_styles' ) ) {
return;
}

$global_styles = gutenberg_get_global_styles();

$found_webfonts = array();

// Look for fonts in block presets...
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking: How would we feel about providing a little more context for those that might be unfamiliar with global styles?

// Global styles can specify typography for individual blocks,
// so look for fonts in block presets...


// Global styles can specify typography for elements like headings, paragraphs, etc.,
// so look for fonts in HTML element presets...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's more explicit, for sure! I'm curious if this is necessary since it kind of goes without saying that global styles can specify the typography since we're looking for them in the code?

Copy link
Contributor

@jeyip jeyip Apr 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm curious if this is necessary since it kind of goes without saying that global styles can specify the typography since we're looking for them in the code?

Sure -- for those examples, I see what you're saying. My intention is to ultimately provide clarity for others by adding context about business use cases, especially since this code will live in Jetpack for the time being ( where folks might not be as familiar with the global styles feature ).

Does that make sense? Maybe we can think about different, more meaningful wording.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. Sure, we can think of something.

if ( isset( $global_styles['blocks'] ) ) {
foreach ( $global_styles['blocks'] as $setting ) {
$font_slug = self::extract_font_slug_from_setting( $setting );

if ( $font_slug ) {
$found_webfonts[ $font_slug ] = 1;
}
}
}

// Look for fonts in HTML element presets...
if ( isset( $global_styles['elements'] ) ) {
foreach ( $global_styles['elements'] as $setting ) {
$font_slug = self::extract_font_slug_from_setting( $setting );

if ( $font_slug ) {
$found_webfonts[ $font_slug ] = 1;
}
}
}

// Check if a global typography setting was defined.
$font_slug = self::extract_font_slug_from_setting( $global_styles );

if ( $font_slug ) {
$found_webfonts[ $font_slug ] = 1;
}

return array_keys( $found_webfonts );
}

}
11 changes: 11 additions & 0 deletions projects/packages/global-styles-webfonts-introspector/tests/php/bootstrap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php
/**
* Bootstrap.
*
* @package automattic/
*/

/**
* Include the composer autoloader.
*/
require_once __DIR__ . '/../../vendor/autoload.php';
4 changes: 4 additions & 0 deletions .../plugins/jetpack/changelog/update-instrospect-google-fonts-instead-of-enqueueing-them-all
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: enhancement

Introspect Google fonts instead of enqueueing them all
1 change: 1 addition & 0 deletions projects/plugins/jetpack/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"automattic/jetpack-constants": "1.6.x-dev",
"automattic/jetpack-device-detection": "1.4.x-dev",
"automattic/jetpack-error": "1.3.x-dev",
"automattic/jetpack-global-styles-webfonts-introspector": "0.1.x-dev",
"automattic/jetpack-google-fonts-provider": "0.2.x-dev",
"automattic/jetpack-heartbeat": "1.4.x-dev",
"automattic/jetpack-identity-crisis": "0.8.x-dev",
Expand Down
46 changes: 45 additions & 1 deletion projects/plugins/jetpack/composer.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading