Block API: Support optional block category #22192
Conversation
aduth
added
[Feature] Block API
aduth
requested review from
ajitbohra,
chrisvanpatten,
ellatrix,
mkaz,
TimothyBJacobs and
youknowriad
as code owners
| itemsPerCategory[ | ||
| category === defaultCategory | ||
| ? undefined | ||
| : category.slug | ||
| ]; |
There was a problem hiding this comment.
This works, but it's a bit strange, because it's assuming there's a string key undefined in itemsPerCategory, i.e.
itemsPerCategory = {
// ...
undefined: [ /* ... */ ],
};
| - `Array<Object>`: Block categories. | ||
| - `Array<WPBlockTypeCategory>`: Block categories. | ||
|
|
||
| <a name="getCategory" href="#getCategory">#</a> **getCategory** |
There was a problem hiding this comment.
Should probably be consistent here with getDefaultCategory, if it's kept in the implementation. There was some prior concern about including this at all, since these singleton methods are discouraged. In any case, they should either both be included, or both be omitted, depending if the current implementation stays.
| 'category' in settings && | ||
| ! some( select( 'core/blocks' ).getCategories(), { |
There was a problem hiding this comment.
I guess to support the use-case described at #19279 (comment), we probably want to make sure that if a block type attempts to register with a category which doesn't actually exist, it should be treated the same as if it were not assigned a category at all. Rather than as it exists here currently, where an error will occur.
|
Size Change: +152 B (0%) Total Size: 824 kB
ℹ️ View Unchanged
|
| @@ -20,7 +20,26 @@ import { combineReducers } from '@wordpress/data'; | |||
| import { __ } from '@wordpress/i18n'; | |||
|
|
|||
| /** | |||
| * Module Constants | |||
| * @typedef WPBlockTypeCategory | |||
There was a problem hiding this comment.
I'm on the fence about whether to call this WPBlockTypeCategory or WPBlockCategory. I think technically WPBlockTypeCategory is the more accurate name†, but (a) it's verbose and (b) we're already pretty consistent elsewhere in this module with calling things "block something" where we actually mean "block type something"
("block collections", "block names", "block styles", etc).
See also: #19279 (comment)
† Accuracy as in: Consider that we don't call labels of register_post_type as "post labels". It would be strange to think about it this way, as it would be very easy to confuse this as "the labels of a post", which is meaningless. That's why the function is called get_post_type_labels, and not get_post_labels.
| blockTypes={ filter( blockTypes, { | ||
| category: category.slug, | ||
| } ) } | ||
| blockTypes={ filter( | ||
| blockTypes, | ||
| ( blockType ) => | ||
| blockType.category === | ||
| ( category === defaultCategory | ||
| ? undefined | ||
| : category.slug ) | ||
| ) } |
There was a problem hiding this comment.
Fun fact: This shorthand object predicate form for Lodash methods does not allow you to filter for undefined values, unless the property actually exists and is undefined. The function form works either way.
const blockTypes = [ { name: 'example' } ];
lodash.filter( blockTypes, { category: undefined } );
// [] 😕
const blockTypes = [ { name: 'example', category: undefined } ];
lodash.filter( blockTypes, { category: undefined } );
// [ {...} ] 🤷♂️ (we can't rely on it)
| 'title' => __( 'Uncategorized' ), | ||
| ); | ||
|
|
||
| return $categories; |
There was a problem hiding this comment.
What's the reasoning for actually registering a default category instead of just having a nullable category and just group blocks with a "null" or inexistant category together in the inserter? It might be simpler and avoids having to define a "default category"?
There was a problem hiding this comment.
What's the reasoning for actually registering a default category instead of just having a nullable category and just group blocks with a "null" or inexistant category together in the inserter? It might be simpler and avoids having to define a "default category"?
This is mentioned in the original comment:
- "Uncategorized" may not need to be an actual block category, but instead handled in user interfaces where absence of category is handled.
- This may actually be good! I'm only now considering it as I write the pull request comment. I do foresee a couple minor downsides.
- Downside: It becomes a risk for duplication and syncing issues if each handling of the "Uncategorized" behavior needs to behave consistently. For example, both the Block Inserter and Block Manager would separately need to implement a (hopefully consistent) "Uncategorized" label. "Hoping" is always a risk, and we could avoid it by treating the default category as a specially-handled case.
It may also require a bit more "explicit" handling in the components where the groupings are arranged, since it wouldn't be able to be treated just the same as any other category.
In the end though, it still may be the simplest option here 🤷 Worth exploring at least.
There was a problem hiding this comment.
What's the reasoning for actually registering a default category instead of just having a nullable category and just group blocks with a "null" or inexistant category together in the inserter? It might be simpler and avoids having to define a "default category"?
Still in progress, but mostly completed, and seems quite a bit simpler indeed: #22280
|
Superseded by #22280 |
Related (partially extracted from): #19279
This pull request seeks to allow for the omission of a category for a registered block type. The effort at #19279 has demonstrated a need to be able to anticipate cases where block types may be registering to categories which do not exist. The requirement of a block category can be seen as unnecessarily restrictive, especially in contrast with many other block settings which are not required (icon, etc), and in observance of the many unit tests which were difficult to implement due to the need to satisfy this arbitrary requirement.
Note: This is being submitted as "In Progress" in order to invite early feedback. It is largely functionally complete, but implementation details may be subject to change.
I considered that there are many ways that this could be approached:
undefinedvs.string), which would not be possible if we forcibly assign a category.getBlockTypeselector.slug,title), a "default" category could be assigned by a separate propertyisDefault, so that there wouldn't need to be separate state betweencategoriesanddefaultCategory.isDefaultwould always be assigned as a boolean, which becomes cumbersome to either normalize or require from a consumer to always passisDefault: falsefor every non-default category.defaultBlockName.Implementation Notes:
Currently, the proposed implementation is as follows:
core/blocks, and can be selected bygetDefaultCategorycategory: undefinedcategory: undefinedas intending to map to the default category