Add: action to set category icon; Fix: Managing block categories documentation

[jorgefilipecosta]

Closes: #11594

The documentation of "Managing block categories" was wrong as on the server we can not set SVG icons. The server would not be able to use our SVG component.
This PR fixes the documentation and adds a new action that allows updating the icon on the front end in a simple way.

How has this been tested?

Add the following code to functions.php:
function my_plugin_block_categories( $categories, $post ) {
	if ( $post->post_type !== 'post' ) {
		return $categories;
	}
	return array_merge(
		$categories,
		array(
			array(
				'slug' => 'my-category',
				'title' => __( 'My category', 'my-plugin' ),
			),
		)
	);
}
add_filter( 'block_categories', 'my_plugin_block_categories', 10, 2 );

Execute the following code block on the browser console:

(function() {
	var el = wp.element.createElement;
	var SVG = wp.components.SVG;
	var circle = el( 'circle', { cx: 10, cy: 10, r: 10, fill: 'red', stroke:	'blue', strokeWidth: '10' } );
	var svgIcon = el( SVG, { width: 20, height: 20, viewBox: '0 0 20 20'},	circle);
	wp.blocks.updateCategory( 'my-category', { icon: svgIcon } );
} )();
( function() {
	const { registerBlockType } = wp.blocks;
	const { createElement: el } = wp.element;
	const { InnerBlocks } = wp.editor;

	registerBlockType( 'block/simple-block', {
		title: 'Test Simple Block',
		icon: 'carrot',
		category: 'my-category',

		edit() {
			return el( 'div', { style: { outline: '1px solid gray', padding: 5 } },
				el( 'p', {}, 'ok' )
			);
		},

		save() {
			return el( 'div', { style: { outline: '1px solid gray', padding: 5 } },
				el( 'p', {}, 'ok' )
			);
		},
	} );
} )();

Verify a circle is the icon of "My category"

[youknowriad]

I'd personally prefer a addCategory action instead of this. This actions is targetting the issue directly but I think the issue is not about allowing to edit the icon but more about allowing to set the icon as SVG from the server. I expect some work around this in phase2 (because it's the same issue for blocks).

So instead of offering a way to edit the icon, I'd prefer if we offer a way to add a category without requiring any filter on the backend.

Is this not a viable option?

[jorgefilipecosta]

Hi @youknowriad,
I think we made the categories changeable on the server because the blocks may be registered on the server.
I thought we may have timing issues if a block registered on the server, used a category added on the client (the add code may not yet have been executed when the block is registered). So I end up in this approach where the category is still managed on the server but we allow updating the icon with client code so SVG's may be open.
Also, the category seems important data that the server should be aware of, so I was not totally comfortable documenting a method that allows categories being added to the client only. Technically this is already possible using the setCategories action but it is not something we recommend anywhere in our docs.
This approaches documents a way to add categories on the server, and then just the icon is updated on the client. I think updating the icon on the client is fine and could be seen as applying a skin and allows a plugin for example change all the default icons. The icon is not information important to the server while the category may end up being important.

That said I can give a try to an add action make some tests and if things seem fine to update this PR, let me know if you think applying this change is worth it.

[gziolo]

What if we limit this PR to offering a store action which updates a category on the client instead? What if someone would want to update the name of the category?

const icon = <SVG ....</SVG>;
dispatch( 'core/blocks' ).updateCategory( slug, { icon } );

I personally don't see any value in offering all those action wrappers which don't have any additional logic but forces us to have the same functionality documented twice. At the same time, it introduces the confusion for plugin developers because there are 2 ways to perform the same task. I think we had getCategories in the past which wasn't using store at all. When we refactored code we didn't remove it which in retrospect should happen in my opinion.

[jorgefilipecosta]

Hi @gziolo, I updated this PR to follow your suggestion.

[gziolo]

Should it be called category for consistency with setCategories action?

[gziolo]

It might need to be tweaked as follows:

To use the SVG icon with the category ...

@tofumatt can you help? :)

[gziolo]

Space is missing after opening parenthesis.

[gziolo]

There is some whitespace issues after stroke:.

[gziolo]

wp.blocks.updateCategory should be wrapped in backticks so it stands out.

[gziolo]

Should categories be stored in an object keyed by the slug? It would simplify update operations. It might negatively influence selectors though.

[gziolo]

@jorgefilipecosta thanks for updating this PR. It looks like it needs to be rebased now because docs structure got an update.

I think it all looks good. I would double check whether we can simplify the reducer after changes introduced. See my comment #11640 (comment).

[jorgefilipecosta]

Hi @gziolo I applied most of your feedback, the missing change was the refactor to the state structure. I'm not sure if it is something, it seems linke a bigger change (involves updating selectors and most of existing category test cases) and if doing it I would prefer to do it in a separate PR to make sure this PR does not becomes unnecessarly huge.

[gziolo]

I tested it and it works great. I tested the following:

• update the existing category with the snippet provided in the PR
• update the existing category again
• update non-existing category - it gets ignored

There is a slight delay until this change will be reflected in the UI when the inserter is opened. It might be nice to open a follow-up PR where we include categories in one of the memoized selectors which controls re-renders for the inserter. I assume it is the reason why it doesn't get updated immediately.

[gziolo]

I'm not sure if it is something, it seems linke a bigger change (involves updating selectors and most of existing category test cases) and if doing it I would prefer to do it in a separate PR to make sure this PR does not becomes unnecessarly huge.

I anticipated it, fine to look at it later if it makes code easier to read for other observers, too.

[youknowriad]

There is a slight delay until this change will be reflected in the UI when the inserter is opened. It might be nice to open a follow-up PR where we include categories in one of the memoized selectors which controls re-renders for the inserter. I assume it is the reason why it doesn't get updated immediately.

This is probably related to the fact that we don't use the selectors to retrieve the categories but use the getCategories global function instead. We should change that when possible, it could be a follow-up.

I think we should hold on merging this PR for the moment until we get some clarity on the need or not of an RC2

[gziolo]

This is probably related to the fact that we don't use the selectors to retrieve the categories but use the getCategories global function instead. We should change that when possible, it could be a follow-up.

Yeah, we should kill that usage for wp.blocks.getCategories sooner than later. The same applies for wp.blocks.getBlockTypes :)