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

Add default-spacing-sizes and default-font-sizes options for classic themes #62252

Merged
merged 11 commits into from
Jun 4, 2024

Conversation

ajlende
Copy link
Contributor

@ajlende ajlende commented Jun 3, 2024

What?

Fixes an issue where defaults were generated in classic themes when they should not have been.

Why?

Bug fix for #58409 and #61842.

How?

Set defaultFontSizes and defaultSpacingSizes to false for classic themes when they have fontSizes or spacingSizes defined, respectively.

Additionally, since it was just a couple extra lines, I added the editor-spacing-sizes, default-font-sizes, and default-spacing-sizes theme supports for parity with other presets.

Testing Instructions

  1. In a classic theme add theme support for editor-font-sizes that match the default slugs. (The default slugs are: small, medium, large, x-large, and xx-large.)
  2. See that the default font sizes are not generated, and the slugs that matched the default slugs show the theme support configuration.
  3. Add theme support for default-font-sizes and see that the default font sizes are generated and they override the theme font sizes.
function my_theme_setup() {
	// Step 1
	add_theme_support(
		'editor-font-sizes',
		array(
			array(
				'name'      => __( 'Small' ),
				'size'      => 19.5,
				'slug'      => 'small',
			),
			array(
				'name'      => __( 'Normal' ),
				'size'      => 22,
				'slug'      => 'normal',
			),
			array(
				'name'      => __( 'Large' ),
				'size'      => 36.5,
				'slug'      => 'large',
			),
			array(
				'name'      => __( 'Huge' ),
				'size'      => 49.5,
				'slug'      => 'huge',
			),
		)
	);
	// Step 2
	add_theme_support( 'default-font-sizes' );
}
add_action( 'after_setup_theme', 'my_theme_setup' );
:root {
	// Step 1
	--wp--preset--font-size--small: 19.5px;
	--wp--preset--font-size--medium: 20px;
	--wp--preset--font-size--large: 36.5px;
	--wp--preset--font-size--x-large: 42px;
	--wp--preset--font-size--normal: 22px;
	--wp--preset--font-size--huge: 49.5px;

	// Step 2
	--wp--preset--font-size--small: 13px;
	--wp--preset--font-size--medium: 20px;
	--wp--preset--font-size--large: 36px;
	--wp--preset--font-size--x-large: 42px;
	--wp--preset--font-size--normal: 22px;
	--wp--preset--font-size--huge: 49.5px;
}

The same should work for editor-spacing-sizes and default-spacing-sizes.

function my_theme_setup() {
	// Step 1
	add_theme_support(
		'editor-spacing-sizes',
		array(
			array(
				'name' => __( '3X-Small' ),
				'size' => '10px',
				'slug' => '10',
			),
			array(
				'name' => __( 'Medium' ),
				'size' => '50px',
				'slug' => '50',
			),
			array(
				'name' => __( 'Large' ),
				'size' => '60px',
				'slug' => '60',
			),
			array(
				'name' => __( '3X-Large' ),
				'size' => '90px',
				'slug' => '90',
			),
		)
	);
	// Step 2
	add_theme_support( 'default-spacing-sizes' );
}
add_action( 'after_setup_theme', 'my_theme_setup' );
:root {
	// Step 1
	--wp--preset--spacing--20: 0.44rem;
	--wp--preset--spacing--30: 0.67rem;
	--wp--preset--spacing--40: 1rem;
	--wp--preset--spacing--50: 50px;
	--wp--preset--spacing--60: 60px;
	--wp--preset--spacing--70: 3.38rem;
	--wp--preset--spacing--80: 5.06rem;
	--wp--preset--spacing--10: 10px;
	--wp--preset--spacing--90: 90px;

	// Step 2
	--wp--preset--spacing--20: 0.44rem;
	--wp--preset--spacing--30: 0.67rem;
	--wp--preset--spacing--40: 1rem;
	--wp--preset--spacing--50: 1.5rem;
	--wp--preset--spacing--60: 2.25rem;
	--wp--preset--spacing--70: 3.38rem;
	--wp--preset--spacing--80: 5.06rem;
	--wp--preset--spacing--10: 10px;
	--wp--preset--spacing--90: 90px;
}

Testing Instructions for Keyboard

Screenshots or screencast

@ajlende ajlende added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Jun 3, 2024
@ajlende ajlende self-assigned this Jun 3, 2024
Copy link

github-actions bot commented Jun 3, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ajlende <[email protected]>
Co-authored-by: carolinan <[email protected]>
Co-authored-by: andrewserong <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

github-actions bot commented Jun 3, 2024

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.6/block-editor.php
❔ lib/class-wp-theme-json-gutenberg.php
❔ lib/class-wp-theme-json-resolver-gutenberg.php
❔ lib/load.php
❔ phpunit/class-wp-theme-json-test.php

@ajlende ajlende requested a review from nerrad as a code owner June 3, 2024 23:29
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

github-actions bot commented Jun 4, 2024

Flaky tests detected in 2dcd295.
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/9369758211
📝 Reported issues:

@carolinan
Copy link
Contributor

I am trying to follow the logic. Reading this it sounds to me like the default font sizes will be disabled by default? So the font sizes will not be selectable in the editor unless the theme is updated to set it to true?

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

I believe this is testing nicely for me. Just double-checking that when I test with Twenty Twenty theme, the after here is the intention as that theme currently sets 'editor-font-sizes' with its own values for small, normal, large, and larger sizes. So prior to this PR, using the unexpected extra sizes, we'd get the following mismatched sizes:

Before

image

But with this PR applied (and no other changes to the theme), then we get the correct sizes that are balanced to the theme:

After

image

If I then go in and add add_theme_support( 'default-font-sizes' ); to the theme, then I can get to the Before image above again. I haven't had a close look at the code yet, but behaviour-wise, this is feeling good to me so far 👍

Copy link
Contributor

@andrewserong andrewserong 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! All testing well:

✅ If the theme sets no editor font sizes and doesn't opt into default font sizes, then default font sizes are available.
✅ If the theme sets editor font sizes but not default font sizes, no default font sizes are available.
✅ If the theme sets both, default font sizes will override.
✅ Same as above for spacing sizes.

I agree the logic might look a bit tricky to follow, but in practice, I think this strikes the right balance, and I haven't been able to fault it while testing, and it resolves a current issue for classic themes such as Twenty Twenty.

This LGTM 🚀, but do feel free to seek a second opinion if there is any doubt about the feature.

The only thing to change from my perspective is a typo in the docs.

@carolinan
Copy link
Contributor

I am not able to reproduce the missing font sizes in classic themes (+ the JavaScript error) with Gutenberg active, with or without this PR; so I can't test this:
✅ If the theme sets no editor font sizes and doesn't opt into default font sizes, then default font sizes are available.

I want to clarify that I don't mean to block the approval of this PR.

pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Jun 4, 2024
See #6616.
See also the original Gutenberg PRs:
* WordPress/gutenberg#58409
* WordPress/gutenberg#61328
* WordPress/gutenberg#61842
* WordPress/gutenberg#62199
* WordPress/gutenberg#62252

Fixes #61282.

Props ajlende, talldanwp, ramonopoly, ellatrix.



git-svn-id: https://develop.svn.wordpress.org/trunk@58328 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Jun 4, 2024
@ellatrix ellatrix added 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 Jun 4, 2024
github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this pull request Jun 4, 2024
@ajlende ajlende merged commit 4c40ec2 into trunk Jun 4, 2024
64 checks passed
@ajlende ajlende deleted the fix/classic-theme-default-font-spacing-sizes branch June 4, 2024 17:12
@github-actions github-actions bot added this to the Gutenberg 18.6 milestone Jun 4, 2024
westonruter added a commit that referenced this pull request Jun 4, 2024
…dd/on-async-directives

* 'trunk' of https://github.com/WordPress/gutenberg: (72 commits)
  Top toolbar: fix half a pixel artifacting of the bottom border. (#62225)
  Fix UI order for theme.json spacing sizes (#62199)
  Chore: Simplify a padding style on global styles. (#62291)
  Editor: Avoid remounts of `DocumentBar` (#62214)
  Add `default-spacing-sizes` and `default-font-sizes` options for classic themes (#62252)
  Editor: Cleanup styles and classnames (#62237)
  Scripts: Pin the @wordpress/scripts version to a version supported by WP 6.5 (#62234)
  Documentation: Better changelogs for the JSX transform upgrade (#62265)
  Chore: Simplify a padding style on dataviews. (#62276)
  MediaUpload: Remove dialog markup on close (#62168)
  Tabs: Prevent accidental overflow in indicator (#61979)
  Make edit site pagination buttons accessibly disabled. (#62267)
  Fix: Remove unused code from dataviews styles. (#62275)
  Re-enable React StrictMode (#61943)
  Inserter: Return the same items when the state and parameters don't change (#62263)
  Update instances of text-wrap: pretty to fall back to balance (#62233)
  Update: Slotfill documentation samples (links, code, and rephrase). (#62271)
  Try: Fix mover positioning. (#62226)
  [Mobile] - Image corrector - Check the path extension is a valid one (#62190)
  Update: Block styles documentation.
  ...
ellatrix pushed a commit that referenced this pull request Jun 11, 2024
…sic themes (#62252)

Co-authored-by: ajlende <[email protected]>
Co-authored-by: carolinan <[email protected]>
Co-authored-by: andrewserong <[email protected]>
ellatrix pushed a commit that referenced this pull request Jun 11, 2024
…sic themes (#62252)

Co-authored-by: ajlende <[email protected]>
Co-authored-by: carolinan <[email protected]>
Co-authored-by: andrewserong <[email protected]>
patil-vipul pushed a commit to patil-vipul/gutenberg that referenced this pull request Jun 17, 2024
…sic themes (WordPress#62252)

Co-authored-by: ajlende <[email protected]>
Co-authored-by: carolinan <[email protected]>
Co-authored-by: andrewserong <[email protected]>
@ellatrix ellatrix 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 Jun 18, 2024
@ellatrix
Copy link
Member

This was cherry-picked to the wp/6.6 branch.

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 [Type] Bug An existing feature does not function as intended
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants