Skip to content
This repository was archived by the owner on Feb 17, 2025. It is now read-only.

Conversation

@pbking
Copy link
Contributor

@pbking pbking commented Dec 12, 2022

I'm not sure that this works like it's supposed to but here's an idea for a fix to allow Jetpack curated fonts to work with blockbase.

This removes the "filter out all jetpack fonts" and instead just filters out those fonts that Blockbase is already providing.

I THINK that this was previously attempted but rejected because it was causing fonts from Blockbase not to work. That doesn't seem to be the case any more (I think) so hopefully something has changed since then to allow this to work OK.

Fixes: #6765

To test you'll need to have this running in an environment where Jetpack is providing fonts. WPCOM is the easiest place to test this. With this branch you should see additional Fonts that Jetpack is providing (at the time of my testing those were the following:

image

NOTE: Remember to update blockbase-premium as well!

@pbking pbking requested review from a team and simison December 12, 2022 18:53
@simison
Copy link
Member

simison commented Dec 15, 2022

One of the problems I'm observing is that fonts from different sources end up in unexpected alphabetical order:

Screenshot 2022-12-15 at 12 44 49

Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

I'm approving this based on;

  • ✅ Code looks fine
  • ✅ Tests well in the editor
  • 🟨 I'm not able to get most theme fonts work in front of the site with or without this PR; loading different fonts with themes/plugins is so messy that it's a bit hard to track where the problem is and it doesn't seem to be entirely specific to this PR. So please double check. Suspecting its due to localhost setup.
  • 🟨 I'm not familiar with theme test process at all so I just copied the changed PHP lines to the theme pulled from .org repo, which might affect things.

Although I think we should still just disable Blockbase fonts entirely when Jetpack fonts module is active instead of doing this kind of "merge both sources" stuff, as Jetpack fonts:
- works with all themes and experience is hence more consistent especially considering theme switches,
- is a single source that's easier to keep up to date. Blockbase seems to already have outdated fonts list. Current either/or setup is way too complex, and this PR is actually making it even more complicated (merged lists) although it does fix an issue.
- Jetpack fonts are now loaded from WP.com instead of Google

Copy link
Contributor

@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.

When testing I noticed a couple issues that gave me pause:

  1. The site editor did not appear to register any additional fonts from Jetpack. Which fonts should I expect to see?

Screenshot 2022-12-19 at 10 38 59 AM

  1. In the post editor, some additional fonts were registered but appended to the bottom of the list which feels weird, should this list be sorted somehow?

Screenshot 2022-12-19 at 10 39 38 AM

@simison
Copy link
Member

simison commented Dec 19, 2022

The site editor did not appear to register any additional fonts from Jetpack. Which fonts should I expect to see?

See list of additional fonts (and I think there were some from previous additions)

Make also sure to enable Jetpack fonts module;

  1. Have latest Jetpack from dev or production. Dev has a few extra fonts (above PR)
  2. connect jetpack
  3. go to jetpack dashboard
  4. at the footer, modules
  5. Enable Fonts module

@simison
Copy link
Member

simison commented Dec 19, 2022

Those extra fonts seem to indeed be from Jetpack. See the list.

Copy link
Contributor

@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 works as described, tested in both the editors and front-end.

Although I think we should still just disable Blockbase fonts entirely when Jetpack fonts module is active instead of doing this kind of "merge both sources" stuff, as Jetpack fonts:

I agree and looked into this a bit, at the moment there is no API for unregistering fonts AFAIK. A bit more context here:

WordPress/gutenberg#40288

Ideally we would unregistering them programmatically if Jetpack fonts module is active, or not register them at all. We could rewrite the font registration + enqueuing in Blockbase, but I think that should be explored in a follow up. For now this PR allows the fonts list to be extended.

@jffng jffng merged commit 5be05d3 into trunk Dec 19, 2022
@jffng jffng deleted the blockbase-allow-jetpack-fonts branch December 19, 2022 17:27
madhusudhand added a commit that referenced this pull request Dec 20, 2022
madhusudhand added a commit that referenced this pull request Dec 20, 2022
@simison
Copy link
Member

simison commented Dec 20, 2022

Regarding revert (#6788) I've been testing the changeset again on .com simple sites and can't replicate the "missing fonts" issue. Any hunch what I might be missing? Tested with Blockbase child theme "Appleton".

$filtered_list[] = $jetpack_font_family;
}
}
return $filtered_list;
Copy link
Member

@simison simison Dec 20, 2022

Choose a reason for hiding this comment

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

Just to make this function easier to follow, something like this might make sense:

  • Renamed vars, they weren't clear always where the data was coming from
  • Condensed, hopefully without being too clever. Instead of foreach use array_column
  • Considered using array_filter to remove second foreach but this felt still more readable.
function blockbase_filter_jetpack_google_fonts_list( $jetpack_fonts ) {
	$theme_fonts         = collect_fonts_from_blockbase();
	$theme_font_families = array_column( $theme_fonts, 'name' );
	$filtered_list = array();

	// If the Jetpack font isn't in theme already, let Jetpack register it
	foreach ( $jetpack_fonts as $jetpack_font_family ) {
		if ( ! in_array( $jetpack_font_family, $theme_font_families, true ) ) {
			$filtered_list[] = $jetpack_font_family;
		}
	}

	return $filtered_list;
}

@simison
Copy link
Member

simison commented Dec 20, 2022

I agree and looked into this a bit, at the moment there is no API for unregistering fonts AFAIK. A bit more context here:

@jffng BTW there now is new wp_deregister_font_family() API

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Blockbase: Defer to Jetpack Fonts

4 participants