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: Simple way to register block styles. #16356

Merged
merged 1 commit into from Jul 26, 2019

Conversation

@jorgefilipecosta
Copy link
Member

commented Jun 28, 2019

Description

Part of: #7551

This PR adds a simple mechanism for theme and plugin developers to register block styles using only PHP function calls to abstract the existing JavaScript mechanism.

How has this been tested?

I added the following code to the functions.php file of the active theme:

add_action(
	'enqueue_block_assets',
	function() {
		wp_register_style( 'myguten-style', get_template_directory_uri() . '/custom-style.css' );
	}
);

register_block_style(
	'core/quote',
	array(
		'name'         => 'fancy-quote',
		'label'        => 'Fancy Quote',
		'style_handle' => 'myguten-style',
	)
);

register_block_style(
	'core/quote',
	array(
		'name'         => 'not-fancy-quote',
		'label'        => 'Not Fancy Quote',
		'inline_style' => '.wp-block-quote.is-style-not-fancy-quote { color: blue; }',
	)
);

register_block_style(
	'core/quote',
	array(
		'name'         => 'unregistered-fancy-quote',
		'label'        => 'Unregistered Fancy Quote',
		'inline_style' => '.wp-block-quote.is-style-unregistered-fancy-quote { color: orange; }',
	)
);
unregister_block_style( 'core/quote', 'unregistered-fancy-quote' );

I created a custom-style.css file in the theme directory with the following contents:

.wp-block-quote.is-style-fancy-quote {
	color: tomato;
}

I loaded the block editor; I verified quote block has Fancy Quote and Not Fancy Quote, and both styles are correctly applied to the editor and the frontend.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

function myguten_stylesheet() {
wp_enqueue_style( 'myguten-style', get_template_directory_uri() . '/custom-style.css' );
}

this should be wp_register_style and not enqueue right?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

Ohhh I understand the proposed API. I think I'd prefer this instead:

wp_register_style( 'myguten-style', get_template_directory_uri() . '/custom-style.css' );

gutenberg_register_block_style(
	'core/quote',
	array(
		'name' => 'fancy-quote',
		'label' => 'Fancy Quote'
	),
	'myguten-style'
);
@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

I also wonder if we should consolidate the handle in the options array instead:

gutenberg_register_block_style(
	'core/quote',
	array(
		'name' => 'fancy-quote',
		'label' => 'Fancy Quote',
                'style' => 'myguten-style'
	)
);

we could also support the inline variation using the same API

gutenberg_register_block_style(
	'core/quote',
	array(
		'name' => 'fancy-quote',
		'label' => 'Fancy Quote',
                'inline_style' => '.wp-block-quote.is-style-not-fancy-quote {	color: blue; }
	)
);

@jorgefilipecosta jorgefilipecosta force-pushed the add/simple-way-to-register-block-styles branch from f2d105a to 759d3aa Jul 1, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

Hi @youknowriad the API was updated to follow your suggestion. "The How has this been tested?" section was updated with the tests done for this API.

@jorgefilipecosta jorgefilipecosta force-pushed the add/simple-way-to-register-block-styles branch 2 times, most recently from 714a30a to 946e0ab Jul 1, 2019

unset( $style_properties[ 'style' ] );
add_action(
'enqueue_block_assets',
function() use ( $style_handle ) {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 1, 2019

Contributor

This is probably not compatible with php 5.2. If we keep it, we should update the minimum version of the plugin to 5.2 (which I think could be considered)

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jul 2, 2019

Author Member

Probably upgrading minimum plugin version is the way to go. As an aside, I'm probably missing something but with PHP 5.2 the only way to express this would be using a class where we pass the $style_handle handle and then that class has a function that calls wp_enqueue_style( $this->style_handle ); or using something like create_function which was deprecated. The PHP 5.2 options that come to my mind all seem verbose.

This comment has been minimized.

Copy link
@Soean

Soean Jul 2, 2019

Member

PR for WordPress 5.2 minimum (PHP 5.6) #15809

add_action(
'enqueue_block_assets',
function() use ( $inline_style ) {
wp_add_inline_style( 'wp-block-library', $inline_style );

This comment has been minimized.

Copy link
@aduth

aduth Jul 1, 2019

Member

What if it's not a core block?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 2, 2019

Contributor

What problem would that cause?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 2, 2019

Contributor

I mean yeah attaching to wp-block-library seems weird. Maybe wp-block-editor instead but I don't see any problem resulting from this.

The question is what does it mean to register a block style. Are we registering this style for all block-editor implementations in core, just the post editor..

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 2, 2019

Contributor

(also same question about the script down here)

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jul 2, 2019

Author Member

I think using wp-block-editor would not work because these styles also need to be enqueued on the website frontend.
Ideally, these styles would be loaded after the block styles because, in case of specificity conflict, block styles would be the used ones. Using wp-block-library ensures that this styles load after the core block styles, it is true that for non-core ones that's not the case, I guess we can increase that probability by changing to a lower priority.
For non-core blocks, an option would be to use WP_Block_Type_Registry get the registered block and add use wp_add_inline_style( $block_type->style, $inline_style );. The problem is that I verified and for most cases this style is null. I guess it is only not null if the block was registered with register_block_type PHP function and the style was passed as one of the args -- I think that this not a common way to register blocks and its styles.

Another option would be to register an inline styles handle that does not point to a file:
wp_register_style( 'wp-inline-block-styles', false );
wp_enqueue_style( 'wp-inline-block-styles' );

And then we would use wp_add_inline_style( 'wp-inline-block-styles', $inline_style );

The current option ensures that inline-block styles are loaded when wp-block-library styles are loaded if these styles are enqueued inline-block styles would also not be added, that seemed a good option.
Using a handle just for the inline-block styles also does not seems wrong, but using false for src looks a little "hacky".

Regarding the inline script to register the style, I used wp-blocks as its handle because it calls a function part of wp.blocks.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 18, 2019

Contributor

Thanks for the details, it seems like there's no perfect solution at the moment. Let's ship the first version as is and iterate.

* @param string $block_name Block type name including namespace.
* @param array $style_properties Array containing the properties of the style name, label, style (name of the stylesheet to be enqueued), inline_style (string containing the CSS to be added).
*/
function gutenberg_register_block_style( $block_name, $style_properties ) {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 18, 2019

Contributor

Let's rename this register_block_style because it's a function that is meant to land on Core as well.

@youknowriad
Copy link
Contributor

left a comment

LGTM 👍

@jorgefilipecosta jorgefilipecosta force-pushed the add/simple-way-to-register-block-styles branch 2 times, most recently from 6bf262a to 7c5bdf7 Jul 18, 2019

@swissspidy
Copy link
Member

left a comment

According to the WordPress PHP coding standards, closures must not be passed as filter or action callbacks, as they cannot be removed by remove_action() / remove_filter().

So if this should ever get into WordPress core, I recommend changing this code accordingly. Perhaps by using a custom class-based solution or by having this function extend WP_Block_Type in some way.

@jorgefilipecosta jorgefilipecosta force-pushed the add/simple-way-to-register-block-styles branch 2 times, most recently from 076c33e to 667b70e Jul 25, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

This PR was updated and now does not uses anonymous functions, following the recommendation by @swissspidy. It is ready for a new look.

* @param string $block_name Block type name including namespace.
* @param array $style_properties Array containing the properties of the style name, label, style (name of the stylesheet to be enqueued), inline_style (string containing the CSS to be added).
*/
function register_block_style( $block_name, $style_properties ) {

This comment has been minimized.

Copy link
@swissspidy

swissspidy Jul 25, 2019

Member

How can someone unregister a block style using PHP, when it was registered using PHP?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jul 25, 2019

Author Member

Good question @swissspidy. We have three options to implement unregister functionality:
1 - A unregister_block_style function that uses the global wp_filters to get the class instance used in the filter and then calls remove_action. (seems hacky but should work well).
2 - Register block style stores the WP_Block_Styles_Enqueue_Block_Assets_Action in a global array. unregister_block_style function retrieves the instance from the global array using (block name and style name) and calls remove_action.
3 - Plugin developers pass an instance of WP_Block_Styles_Enqueue_Block_Assets_Action (maybe we should find a better name in that case) to register_block_style and they can save the instance to later call unregister_block_style. This does not allow another plugin to remove a style added as they may not have access to the instance.

It seems using the current register API option 2 is the best, but using a global variable is not ideal.

Another possibility is using a new class BlockStyles with a static method to register and another to unregister. The class could follow the same approach specific in 2 but using an instance variable instead of a global variable.

Any ideas about what approach to follow? Is there an alternative approach that I'm missing and is better than the ones referred?

This comment has been minimized.

Copy link
@swissspidy

swissspidy Jul 25, 2019

Member

Another possibility is using a new class BlockStyles with a static method to register and another to unregister. The class could follow the same approach specific in 2 but using an instance variable instead of a global variable.

👍

This reminds me a lot of WP_Block_Type_Registry, which basically does the same thing.


Now that I think about it...

Perhaps – since this is about adding some information for an existing block type - we could even re-use WP_Block_Type_Registry here. Example (off the top of my head):

function register_block_style( $block_name, $style_properties ) {
	$block = null;

	if ( WP_Block_Type_Registry::is_registered( $block_name ) ) {
		$block = WP_Block_Type_Registry::unregister( $block_name );
	} else {
		$block = new WP_Block_Type( $block_name );
	}

	// This property would need to be added to WP_Block_Type in core eventually.
	$block->style_properties = isset( $block->style_properties ) ? $block->style_properties : array();
	$block->style_properties[] = $style_properties;

	WP_Block_Type_Registry::register( $block_name );
}

// This could be added directly to wp_enqueue_registered_block_scripts_and_styles()
function gutenberg_enqueue_additional_styles() {
	global $current_screen;

	$is_editor = ( ( $current_screen instanceof WP_Screen ) && $current_screen->is_block_editor() );

	$block_registry = WP_Block_Type_Registry::get_instance();
	foreach ( $block_registry->get_all_registered() as $block_name => $block_type ) {
		// Front-end styles.
		if ( $is_editor && ! empty( $block_type->style_properties ) ) {
			foreach( $block_type->style_properties as $properties ) {
				// Do all the sanity checks etc.
				wp_add_inline_script( /* ... */ );
			}
		}
	}
}

add_action( 'enqueue_block_assets', 'gutenberg_enqueue_additional_styles' );

Just thinking out loud here 🙂 Perhaps some other voices from core PHP devs would be helpful here, perhaps from @pento.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jul 25, 2019

Author Member

I'm not sure if we should reuse WP_Block_Type_Registry as styles and blocks are not exactly the same in the client we use different structures. I agree the problem is the same. In php, it should also be possible to add styles to blocks that the server is not even aware they exist (registered with JS) I will probably use a WP_Block_Styles_Registry as a registry for styles, and follow path 2 instead of using a global I will use the styles registry.

@jorgefilipecosta jorgefilipecosta force-pushed the add/simple-way-to-register-block-styles branch 3 times, most recently from c736397 to 7840638 Jul 25, 2019

@jorgefilipecosta jorgefilipecosta force-pushed the add/simple-way-to-register-block-styles branch 2 times, most recently from 686fa1d to 6541dd5 Jul 25, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

This PR was updated to use a block styles registry following some ideas exchanged with @swissspidy, a function that allows unregistering in PHP a previously registered block in PHP was also implemented.

@swissspidy

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

Amazing work @jorgefilipecosta! 🚀 👏

@swissspidy swissspidy requested a review from youknowriad Jul 25, 2019

)
),
$block_name,
wp_json_encode( $client_style_properties )

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 25, 2019

Contributor

Could we pass the right attributes directly array( 'key' => $values ) instead of having to unset properties as it could break if we add new properties in the future.

unset( $client_style_properties['inline_style'] );
}
wp_add_inline_script(

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 25, 2019

Contributor

I feel this should be in a separate action enqueue_block_editor_assets that is specific to the editor (the styles are good right now though). We could also avoid wp_add_inline_script and enqueue a separate inline script that depends on wp-blocks instead.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jul 25, 2019

Author Member

Hi @youknowriad, I updated to use another action enqueue_block_editor_assets for the addition of the script as suggested 👍

Regarding the way to avoid wp_add_inline_script, I'm probably missing something -- I'm not seeing how we can avoid a call to wp_add_inline_script (unless printing our own inline script tags).
We can do something similar to:

	wp_register_script( 'wp-block-styles', false, array( 'wp-blocks' ) );
	wp_add_inline_script( 'wp-block-styles', $inline_script );
	wp_enqueue_script( 'wp-block-styles' );

Registering our own script that does not point to any file, but that seems more hacky and I'm not sure if phpcs will not complain of this approach.

In this approach, we still have a call to wp_add_inline_script, was your idea different from what we have or from this approach I commented?

I performed another side update; now, all the scripts are registered together in a single function and a single inline script. This change saves some bytes on every editor page load.
For the inline styles, it is better not to apply a similar change. Inline-styles are not generated by us they are provided by the plugin/theme developers, having an invalid style in a plugin would invalid other styles.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 25, 2019

Contributor

Registering our own script that does not point to any file, but that seems more hacky and I'm not sure if phpcs will not complain of this approach.

This is exactly what I was suggesting. The advantage is that no matter if wp-blocks is already enqueued or not, it will work (no race conditions).

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 25, 2019

Contributor

I don't see it as crucial though.

@jorgefilipecosta jorgefilipecosta force-pushed the add/simple-way-to-register-block-styles branch 2 times, most recently from 9f49ef9 to 8acaf31 Jul 25, 2019

$inline_script = '( function() {' . "\n";
foreach ( $block_styles as $block_name => $styles ) {
foreach ( $styles as $style_properties ) {
$inline_script .= sprintf(

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 25, 2019

Contributor

very minor: when this pattern is needed (concat + wrap) I tend to use an array (here it would be an array of iniline scripts) and just join and wrap at the end.

Add: Simple way to register block styles.
Co-Authored-By: Pascal Birchler <pascalb@google.com>

@jorgefilipecosta jorgefilipecosta force-pushed the add/simple-way-to-register-block-styles branch from 8acaf31 to a5cce4a Jul 26, 2019

@jorgefilipecosta jorgefilipecosta merged commit 944392d into master Jul 26, 2019

1 of 2 checks passed

Milestone It Milestone It
Details
Travis CI - Pull Request Build Passed
Details

@jorgefilipecosta jorgefilipecosta deleted the add/simple-way-to-register-block-styles branch Jul 26, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

Thank you for the feedback @swissspidy and @youknowriad, all the feedback was applied before the merge. If there is any other possible improvement that you notice feel free to comment and I will open a follow-up PR.

@youknowriad youknowriad added this to the Gutenberg 6.2 milestone Jul 26, 2019

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

Add: Simple way to register block styles (WordPress#16356)
Co-Authored-By: Pascal Birchler <pascalb@google.com>
@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

It would be good to document this new API somewhere :)

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

It would be good to document this new API somewhere :)

Added to my to-do list. I should create a follow-up PR soon :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.