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

[Global Styles]: Add support for nameless font sizes #37410

Merged

Conversation

ntsekouras
Copy link
Contributor

@ntsekouras ntsekouras commented Dec 15, 2021

Very recently in this PR we updated the default font sizes - the new defaults have the slugs (small, medium, large, x-large.

With this PR we allow themes to skip the declaration of name of their fontSizes, if they want to support the defaults. What this means is that a theme can now provide their fontSizes like this:

"fontSizes": [
	{
		"slug": "small",
		"size": "1.3rem"
	},
	{
		"slug": "medium",
		"size": "1.75rem"
	},
	{
		"slug": "large",
		"size": "2rem"
	},
	{
		"slug": "x-large",
		"size": "3rem"
	}
],

** Note the missing name field and that the slugs match the default ones.

Notes

  • If a theme doesn't provide a name for a slug that isn't among the defaults, it will result to having no name with all the bad implications that entails.
  • If a theme uses a slug that is among the defaults, but provides a name, the provided name will be used.

Testing instructions

  1. Add to your theme.json nameless font sizes that use the core default slugs (example above)
  2. Observe that in font size picker the names from the defaults are shown as labels.

@ntsekouras ntsekouras self-assigned this Dec 15, 2021
@ntsekouras ntsekouras marked this pull request as ready for review December 15, 2021 18:04
@ntsekouras ntsekouras added Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Dec 15, 2021
@kjellr
Copy link
Contributor

kjellr commented Dec 15, 2021

This seems to work for me. I tried it alongside WordPress/twentytwentytwo#298 (both with and without name defined for each preset), and they both showed the correct names:

sizes

In the case of Twenty Twenty-Two, I'm not sure we actually need to update our font sizes though. 🤔 Our current five sizes and provided names work just fine with this setup as is:

sizes

I guess the main reason to do it would be to ensure we align with the t-shirt sizes if/when core switches to that.

@jasmussen
Copy link
Contributor

jasmussen commented Dec 15, 2021

Thanks for the PR! I'm not sure what I'm doing wrong, but it wasn't working for me:

sizes

This is with the following in my custom theme.json:

		"typography": {
			"fontSizes": [
				{
					"slug": "small",
					"size": "12px"
				},
				{
					"slug": "medium",
					"size": "14px"
				},
				{
					"slug": "large",
					"size": "18px"
				},
				{
					"slug": "x-large",
					"size": "48px"
				},
				{
					"slug": "xx-large",
					"size": "64px",
					"name": "XX Large"
				}
			],

I feel like there's something I'm messing up with my system, because I'm also seeing the (px) unit next to the title, whereas it seems to be missing in the other screenshots. Is that due to the complex units from TT2?

I guess the main reason to do it would be to ensure we align with the t-shirt sizes if/when core switches to that.

I think "Normal" changing to "Medium" and "Huge" changing to "Extra Large" feel like good reasons to sync up TwentyTwentyTwo as well, since both the defaults and the default theme should showcase best practices.

For the 4 base names, I'd even go so far as to remove the name value from the theme.json, and intentionally fall back to the default names. That is of course, unless I misunderstood how that was meant to work!

@ntsekouras
Copy link
Contributor Author

I'm not sure what I'm doing wrong, but it wasn't working for me:

Hm... this seems to work for me and I don't see something wrong with your fontSizes.

I feel like there's something I'm messing up with my system, because I'm also seeing the (px) unit next to the title, whereas it seems to be missing in the other screenshots. Is that due to the complex units from TT2?

Yes, it due to the complex units.

@jasmussen
Copy link
Contributor

jasmussen commented Dec 15, 2021

I'll give this another spin as soon as I get a moment, but just from my understanding this (with your confirmation), this is an important step forward 👍👍

@jasmussen
Copy link
Contributor

Definitely something with my environment. I'm seeing the correct names now:
names

That's testing with this in my theme.json:

			"fontSizes": [
				{
					"slug": "small",
					"size": "12px"
				},
				{
					"slug": "medium",
					"size": "14px"
				},
				{
					"slug": "large",
					"size": "18px"
				},
				{
					"slug": "x-large",
					"size": "48px"
				},
				{
					"slug": "xxl",
					"size": "64px",
					"name": "XX Large"
				}
			],

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

I would encourage a code review of this, but from a theme.json perspective, thumbs up! Thanks for the work.

@ntsekouras ntsekouras merged commit 5604b23 into trunk Dec 17, 2021
@ntsekouras ntsekouras deleted the add/support-for-nameless-font-sizes-with-core-presets branch December 17, 2021 06:37
@github-actions github-actions bot added this to the Gutenberg 12.3 milestone Dec 17, 2021
ntsekouras added a commit that referenced this pull request Dec 17, 2021
* add support for nameless font sizes

* add test
@ntsekouras
Copy link
Contributor Author

I've cherry picked in wp/trunk here and the core patch is here

--cc @tellthemachines

@tellthemachines
Copy link
Contributor

Thanks @ntsekouras , I'll remove the backport label then.

@tellthemachines tellthemachines removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Dec 20, 2021
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Dec 21, 2021
With nameless font sizes support, themes use the defaults by not declaring the `"name"` setting for their `fontSizes` in `theme.json`.

Backport from Gutenberg WordPress/gutenberg#37410.

Follow-up to [50973], [52049], [52275], [52320].

Props ntsekouras, costdev, hellofromTonya.
Fixes #54640. See #54487.

git-svn-id: https://develop.svn.wordpress.org/trunk@52401 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Dec 21, 2021
With nameless font sizes support, themes use the defaults by not declaring the `"name"` setting for their `fontSizes` in `theme.json`.

Backport from Gutenberg WordPress/gutenberg#37410.

Follow-up to [50973], [52049], [52275], [52320].

Props ntsekouras, costdev, hellofromTonya.
Fixes #54640. See #54487.
Built from https://develop.svn.wordpress.org/trunk@52401


git-svn-id: http://core.svn.wordpress.org/trunk@51993 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott pushed a commit to gMagicScott/core.wordpress-mirror that referenced this pull request Dec 21, 2021
With nameless font sizes support, themes use the defaults by not declaring the `"name"` setting for their `fontSizes` in `theme.json`.

Backport from Gutenberg WordPress/gutenberg#37410.

Follow-up to [50973], [52049], [52275], [52320].

Props ntsekouras, costdev, hellofromTonya.
Fixes #54640. See #54487.
Built from https://develop.svn.wordpress.org/trunk@52401


git-svn-id: https://core.svn.wordpress.org/trunk@51993 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants