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

Register webfonts defined in styles variations #39886

Merged
merged 4 commits into from
Apr 8, 2022
Merged

Register webfonts defined in styles variations #39886

merged 4 commits into from
Apr 8, 2022

Conversation

aristath
Copy link
Member

@aristath aristath commented Mar 30, 2022

What?

Fixes #39702
Registers webfonts defined in variations, as described in the issue.

Why?

This will allow defining webfonts in styles variations.

How?

  • In the editor, all webfonts registered in variations get added
  • On the frontend, we use the merged data, so if the user has selected to use a webfont from a variation, then (and only then) the webfont gets added.

Testing Instructions

Testing instructions available in #39702

@aristath aristath requested a review from kjellr March 30, 2022 10:58
@aristath aristath changed the title Register webfonts defined in variations Register webfonts defined in styles variations Mar 30, 2022
@aristath aristath marked this pull request as draft April 4, 2022 05:24
@aristath aristath marked this pull request as ready for review April 5, 2022 09:47
@youknowriad
Copy link
Contributor

It took me some time to be able to test this :). Should we include a variation with a font file in the "emptytheme" embedded in the Gutenberg plugin to make testing this easier in the future?

Regardless, I'm noticing two bugs:

I have a default theme.json without font family (default) and one variation with a font applied to the whole body.

  • If apply the variation on the editor and save, then reload the frontend, the frontend is still displaying the default font family.
  • In the editor, if I have the variation applied and I save and reload. I'm not able to "reset" the styles to the default theme.json. Basically when I click "reset styles", the variation's font family is still applied somehow.

(Testing in safari)

@kjellr
Copy link
Contributor

kjellr commented Apr 5, 2022

This emptytheme PR works for testing: WordPress/theme-experiments#304

I'm also testing alongside the Twenty Twenty-Two style variations here: WordPress/wordpress-develop#2440

The former works just fine aside from the issues that @youknowriad noted above — I am able to reproduce those. But the Twenty Twenty-Two PR doesn't seem to load any fonts at all. 🤔

If someone could help gut check that for me, I'd appreciate it — I'm unclear if the problem is over on the Twenty Twenty-Two end or in this PR.

@aristath
Copy link
Member Author

aristath commented Apr 8, 2022

If apply the variation on the editor and save, then reload the frontend, the frontend is still displaying the default font family.

Apparently I didn't test after rebasing the PR, and some things have changed 😅
Fixed. It appears that after some recent changes in the webfonts API, now an extra call is required to enqueue the webfont, registering them is not enough.

In the editor, if I have the variation applied and I save and reload. I'm not able to "reset" the styles to the default theme.json. Basically when I click "reset styles", the variation's font family is still applied somehow.

After some debugging, I don't think this is a bug in this PR...
The problem here is that the theme.json file in the emptytheme doesn't have a default typography defined, while the variation does.
If in theme.json I add this (see snippet below) then everything works:

	"styles": {
		"typography": {
			"fontFamily": "sans-serif"
		}
	}

If we want to change that behavior, it will need to be done in a separate PR since it's more generic and not related to the webfonts... it's an overrides/resets issue when a variation has something defined that doesn't exist in the main file.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

This is looking good to me. (I agree the reset bug is separate, I'll look into it).

Also cc @oandregal in case you want to take a look here.

Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

I tested with @kjellr's TT2 variations PR and it works for me

@Addison-Stavlo Addison-Stavlo requested a review from zaguiini April 8, 2022 11:18
@aristath aristath merged commit d14832c into trunk Apr 8, 2022
@aristath aristath deleted the try/39702 branch April 8, 2022 11:41
@github-actions github-actions bot added this to the Gutenberg 13.0 milestone Apr 8, 2022
@bph bph added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Dev Note Requires a developer note for a major WordPress release cycle
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Styles] Webfonts enqueued using styles JSON files are not loaded
7 participants