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

Block Styles: Extend block style variations as mechanism for achieving section styling #6662

Conversation

aaronrobertshaw
Copy link

@aaronrobertshaw aaronrobertshaw commented May 29, 2024

Provide users with the ability to style entire sections of a page without having to tediously reapply the same sets of styles. This will amplify the site-building potential of pattern overrides, zoomed out mode etc.

A full history of the feature can be found in: WordPress/gutenberg#57537

Syncs the changes from: WordPress/gutenberg#57908 & WordPress/gutenberg#62125

Detailed test instructions can be found on the linked Gutenberg PRs.

Some specific things to test include:

  • Registration of block style variations via theme.json partials in /styles and theme style variations
  • Existing block style variations such as Button's Outline style continue to display correctly on the frontend
  • Custom block style variations can be applied in both editors (they won't display properly until the JS changes land)
  • Custom block style variations can be applied to nested blocks in any order (frontend displays fine)
  • Inner elements and block types can be styled for block style variations and display on the frontend
  • Styles applied to individual block instances take precedence on the frontend
  • Unit tests pass
npm run test:php -- --filter Tests_Block_Supports_WpCreateBlockStyleVariationInstanceName
npm run test:php -- --filter Tests_Block_Supports_WpGetBlockStyleVariationNameFromClass
npm run test:php -- --filter WP_Block_Supports_Block_Style_Variations_Test
npm run test:php -- --filter Tests_Theme_wpThemeJsonResolver
npm run test:php -- --filter Tests_Theme_wpThemeJson
npm run test:php -- --filter Tests_Theme_ThemeDir

Trac ticket: https://core.trac.wordpress.org/ticket/61312


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@aaronrobertshaw aaronrobertshaw changed the title Draft backport for section styles Block Styles: Extend block style variations as mechanism for achieving section styling May 29, 2024
@aaronrobertshaw aaronrobertshaw force-pushed the add/section-styling-via-block-styles branch from 0e6a912 to bd79a65 Compare May 29, 2024 09:54
@aaronrobertshaw aaronrobertshaw marked this pull request as ready for review May 29, 2024 09:54
Copy link

github-actions bot commented May 29, 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.

Core Committers: Use this line as a base for the props when committing in SVN:

Props aaronrobertshaw, talldanwp, ramonopoly, noisysocks, peterwilsoncc, isabel_brison, andrewserong.

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

@aaronrobertshaw
Copy link
Author

FYI this backport has been updated to include a bug fix which landed in Gutenberg via WordPress/gutenberg#62125.

@noisysocks
Copy link
Member

Hello! Happy Friday.

Testing first, and then I’ll look at the code.

I followed the testing steps in WordPress/gutenberg#57908 basically to the letter. Thanks for writing them!

I am testing this branch and have the Gutenberg plugin disabled. It’s literally my first time looking at this feature so if I’m doing something wrong just tell me.

When I put this into my functions.php:

register_block_style(
	array( 'core/group', 'core/columns' ),
	array(
		'name'       => 'green',
		'label'      => __( 'Green', 'twentytwentythree' ),
		'style_data' => array(
			'color'    => array(
				'background' => '#4f6f52',
				'text'       => '#d2e3c8',
			),
			'blocks'   => array(
				'core/group' => array(
					'color' => array(
						'background' => '#739072',
						'text'       => '#e3eedd',
					),
				),
			),
			'elements' => array(
				'link' => array(
					'color'  => array(
						'text' => '#ead196',
					),
					':hover' => array(
						'color' => array(
							'text' => '#ebd9b4',
						),
					),
				),
			),
		),
	)
);

I received this notice:

Notice: Function WP_Block_Styles_Registry::register was called incorrectly. Block name must be a string.

I worked around this by replacing the array(...) with just ’core/group'.

I noticed that, with that code above, links within a Group block on the frontend correctly received a #ead196 colour but links within a Group block in the editor remained black. It’s a similar story for the #739072 background on nested Group blocks. I could see that in the frontend but not the editor.

I’m not sure what this means:

Double check whether block style variations with margin styles work correctly when alongside layout global styles.

Do you have an example to test?

When I added a variation by creating a styles/variation-a.json file or by setting styles.blocks.variations in theme.json, I saw that Variation A came up as an option for Group and Columns blocks but selecting that variation didn’t change any of the block’s colours.

Copy link
Member

@ramonjd ramonjd 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 still testing, thought I'd leave a bunch of annoying syntax feedback first 😄

src/wp-includes/class-wp-theme-json-resolver.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
@aaronrobertshaw
Copy link
Author

Thanks for testing @noisysocks and @ramonjd 👍

I received this notice:

Sorry, it looks like I missed rebasing this PR after the backport for extending block style registration. I'll get it updated shortly.

I’m not sure what this means:

Double check whether block style variations with margin styles work correctly when alongside layout global styles.

By that it meant, if a block style variation specifies margin styles, they don't incorrectly override margin styles set by layout styles e.g. the left/right margins on top level blocks etc. Once I sort out the other feedback, I'll circle back and try and come up with a concrete example.

@tellthemachines
Copy link
Contributor

Sorry, it looks like I missed rebasing this PR after the backport for extending block style registration.

Was just coming here to ask for a rebase 😄

@aaronrobertshaw aaronrobertshaw force-pushed the add/section-styling-via-block-styles branch from 5167492 to 3598752 Compare May 31, 2024 04:04
@aaronrobertshaw
Copy link
Author

Ok, this has been rebased and the indentation sorted. Well I hope the indentation/linting issues are sorted, I don't trust my local phpcs/phpcbf config right now 😅

@tellthemachines
Copy link
Contributor

I've been testing this mainly by registering style variations in PHP, both with classic and block themes, and it's working as expected for both ✅
Also checked that existing style variations, whether core or theme-declared, are still working correctly ✅

@aaronrobertshaw
Copy link
Author

I noticed that, with that code above, links within a Group block on the frontend correctly received a #ead196 colour but links within a Group block in the editor remained black. It’s a similar story for the #739072 background on nested Group blocks. I could see that in the frontend but not the editor.

Apologies @noisysocks, I messed up the test instructions. The editor side of this feature requires the JS changes in Gutenberg to function properly there.

I'll update the instructions for others that might be testing this one.

@noisysocks
Copy link
Member

Got it, that makes sense. It think the only issue I'm seeing now then is this one:

When I added a variation by creating a styles/variation-a.json file or by setting styles.blocks.variations in theme.json, I saw that Variation A came up as an option for Group and Columns blocks but selecting that variation didn’t change any of the block’s colours.

Here's a video:

Kapture.2024-05-31.at.14.43.42.mp4

@aaronrobertshaw
Copy link
Author

Appreciate the continued patience here @noisysocks 🙇

The catch with the variation partial used there is that it references css custom properties not present in TwentyTwentyFour. Switching those up to simple hex values might be the best bet for testing.

I'll go and update the original PR's snippets so they don't catch anyone else out.

@ramonjd
Copy link
Member

ramonjd commented May 31, 2024

I'm running through the scenarios from the Gutenberg PR WordPress/gutenberg#57908

So far:

✅ blockTypes blocks are being registered correctly and styles applied on frontend (tested using using theme.json and register_block_style)

Screenshot 2024-05-31 at 2 36 53 pm

✅ existing block style variations on core blocks continue to work

✅ Applying a variation within a block that already has a variation has the correct styles applied

✅ Setting element styles on an individual block instance continues to take precedence

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 testing nicely for me on the site frontend, too. All of the items in the "specific things to test" section worked well for me, with nested styles behaving just as I'd expect:

image

Looking good to me in manual testing!

Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Code's looking pretty good to me! Thanks for adding comprehensive tests ❤️

@ramonjd
Copy link
Member

ramonjd commented May 31, 2024

Just finished a round of specificity testing, e.g., Top-level, blocks, elements and CSS is being applied as I'd expect.

LGTM!

Screenshot 2024-05-31 at 3 26 40 pm

@noisysocks
Copy link
Member

The catch with the variation partial used there is that it references css custom properties not present in TwentyTwentyFour. Switching those up to simple hex values might be the best bet for testing.

I'll go and update the original PR's snippets so they don't catch anyone else out.

Makes sense 👍 thanks!

I changed the var()s in my test code to be hex colours and it works well now.

@noisysocks
Copy link
Member

Committed in r58264.

@noisysocks noisysocks closed this May 31, 2024
*
* @param array $variation Theme.json shaped style variation object.
* @param string $scope Scope to check e.g. theme, block etc.
*
Copy link
Member

Choose a reason for hiding this comment

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

No empty line between "@param" and "@return"

* theme.json partial files by the scope to which they
* can be applied e.g. theme vs block etc.
*
* @param string $scope The scope or type of style variation to retrieve e.g. theme, block etc.
*
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Comment on lines +1225 to +1228
* The options currently supported are:
* - 'scope' that makes sure all style are scoped to a given selector
* - `root_selector` which overwrites and forces a given selector to be used on the root node
* - `skip_root_layout_styles` which omits root layout styles from the generated stylesheet.
Copy link
Member

Choose a reason for hiding this comment

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

Need some space to align the document.

@mukeshpanchal27
Copy link
Member

@noisysocks ☝️

@noisysocks
Copy link
Member

Thanks @mukeshpanchal27, addressed in r58265. Are you interested in fixing phpcs so that it automatically catches this kind of stuff? 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants