Add new function to register a new block category #1732

kuoko · 2017-07-05T13:24:46Z

The new function allows to add new block categories.
I think it's very useful in order to add new blocks and display them under a custom category.
I was inspired by the following ticket: #1352

braders

100% support he need for devs to be able to add categories.

This doesn't allow any control over order though - I wonder what other approaches might allow for this?

It probably also makes sense to be able to remove a category - I'm not sure what should happen to any blocks in that category though?

kuoko · 2017-07-06T09:29:17Z

This doesn't allow any control over order though - I wonder what other approaches might allow for this?

yes. you are right. a solution could be introducing an "order" property (int value) to the category and using that to set the order of rendering.

It probably also makes sense to be able to remove a category - I'm not sure what should happen to any blocks in that category though?

No problem adding a new function to remove an existing category but the only issue I can see at the moment is that all the blocks filed under that category would disappear. Is that the right way of proceeding? That's how it works with WP metaboxes (e.g if we want to remove a specific metabox from wordpress UI). The alternative is to refuse to delete the category until it was empty or to display all the bocks under a generic "No category" label. Any thoughts?

kuoko · 2017-07-11T13:45:59Z

I've added two new function.
The first one allows to set a new "order" property for a block category.
The second function sort the categories array by a provided key (e.g. order, slug, title etc.)
Any feedback is appreciated.

Cheers, Antonio

aduth · 2017-08-03T13:19:03Z

blocks/api/test/categories.js

+/**
+ * External dependencies
+ */
+import { __ } from 'i18n';


Note with #2172, these need to be updated to prefix the dependencies with @wordpress/. You will need to perform a rebase against the latest version of master and apply your changes:

git fetch origin git rebase origin/master

Done.

Cheers, Antonio

codecov · 2017-08-07T14:01:04Z

Codecov Report

Merging #1732 into master will increase coverage by 0.33%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1732      +/-   ##
==========================================
+ Coverage   38.94%   39.27%   +0.33%     
==========================================
  Files         289      289              
  Lines        6948     6986      +38     
  Branches     1276     1286      +10     
==========================================
+ Hits         2706     2744      +38     
  Misses       3566     3566              
  Partials      676      676

Impacted Files	Coverage Δ
blocks/api/categories.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1f7b19...c78d533. Read the comment docs.

aduth · 2017-08-11T19:29:50Z

blocks/api/categories.js

+ *
+ * @returns {Array} Block categories
+ */
+export function sortCategoriesBy( key ) {


Should this be a mutative function, or should it return a new array? If any UI was to show this list, how and when would it be come updated if the sort changes?

My preference might be toward a getter returning a new array: getSortedCategories( sortProperty )

(I guess the same arguments can be made for registering in general, and they have been: #336)

Yes. There isn't any update. It's more a getter, so, I will change the function following your advices in the next commit.

aduth · 2017-08-11T19:31:02Z

blocks/api/categories.js

+ * @return {Array} Block categories
+ *
+ */
+export function registerCategory( cat ) {


Minor: Abbreviations are overrated in my opinion. Don't let a few characters dissuade you from clarity, especially since it's all minified in the end anyways.

Cats are for meowing 😸

Hi, sorry for the delay. I was on holiday. Right, Let's use the full-lenght definition. Moreover, I'm also allergic so better don’t bother cats for this purpose. :)

aduth · 2017-08-11T19:32:34Z

blocks/api/categories.js

+ * @returns {Array} Block categories
+ */
+export function setCategoryOrder( slug, order ) {
+	const pos = findIndex( categories, ( category ) => category.slug === slug );


Lodash's _.find might be closer to what you want, since you don't really care about the index aside from referencing the matched object itself.

aduth · 2017-08-11T19:33:35Z

blocks/api/categories.js

+ *
+ * @returns {Array} Block categories
+ */
+export function setCategoryOrder( slug, order ) {


Would we need the ability to change order after-the-fact like this?

I think this function could be helpful to re-arrange the existing and new block categories. It should be used before the getter (getSortedCategories).

ahmadawais

A few documentation and lingo issues.

ahmadawais · 2017-09-15T13:52:48Z

blocks/api/categories.js

+ * Register a new block category
+ *
+ * @param {Array} category e.g {slug: 'custom', title: __('Custom Blocks')}
+ *


Consistent Documentation: Remove this empty line.

ahmadawais · 2017-09-15T13:53:05Z

blocks/api/categories.js

+ * @param {Array} category e.g {slug: 'custom', title: __('Custom Blocks')}
+ *
+ * @return {Array} Block categories
+ *


Consistent Documentation: Remove this empty line.

ahmadawais · 2017-09-15T13:53:20Z

blocks/api/categories.js

@@ -27,3 +30,105 @@ const categories = [
 export function getCategories() {
 	return categories;
 }
+
+/**
+ * Register a new block category


Consistent Documentation: Add a period at the end.

ahmadawais · 2017-09-15T13:56:23Z

blocks/api/categories.js

+export function registerCategory( category ) {
+	if ( ! category ) {
+		console.error(
+			'The Block category must be defined'


'The block Category must be defined'

Having

b lowercase for block seems right.

C upercase for Category seems right.

Maybe change that everywhere?

kuoko · 2017-09-18T09:28:26Z

Hi @ahmadawais,
thanks for our feedback.
You can find some documentation and lingo tweaks in my last commit.

dmsnell · 2017-10-05T19:32:12Z

blocks/api/categories.js

@@ -11,7 +14,7 @@ import { __ } from '@wordpress/i18n';
 *
 * @var {Array} categories
 */
-const categories = [
+let categories = [


note that there's no need to change this to a let just to allow for push() and splice() and sort() - those are all mutating operations. not a big deal, but just pointing out that we don't need to introduce this change

dmsnell · 2017-10-05T19:41:27Z

blocks/api/categories.js

+		);
+		return;
+	}
+	if ( ! /^[a-z0-9-]+$/.test( category.slug ) ) {


If we're going to define a rule for what category names can be it could be nice to make it explicit somewhere where others can find it without having to dive deep into the code: maybe a constant in the top of the module scope?

/** * @type {RegExp} * @const * * Category names must be a combination of lower-case letters, numbers, and hypens */ const categoryNamePattern = /^[a-z0-9-]+$/ // or… // ^[a-z] - must start with a letter // (?: … )+ - any number of the following (non-capture because it's all part of a whole name) // -* - zero or more hyphens… // [a-z0-9] - …followed by a letter or number // // examples… // 'cats', 'dogs', 'eat-a-burger', 'editor9', 'a---------pause' // // counter-examples… // 'ae-', '-equi', '4u' const categoryNamePattern = /^[a-z](?:-*[a-z0-9])+$/

dmsnell · 2017-10-05T19:44:32Z

blocks/api/categories.js

+		);
+		return;
+	}
+	if ( categories.find( x => x.slug === category.slug ) ) {


note that Array.prototype.some() here might have a more semantic match here with what we're checking: is there any item in this collection which matches the given predicate function?

dmsnell · 2017-10-05T19:46:51Z

blocks/api/categories.js

+		);
+		return;
+	}
+	categories = sortBy( categories, sortProperty );


Might want to verify here that this sort sorts as expected. JavaScript's default sort doesn't always produce the results we want, but when comparing strings we are thankfully given a function to make it more reliable…

const byGivenProperty = ( a, b ) => a[ sortProperty ].localeCompare( b[ sortProperty ] ); categories.sort( byGivenProperty )

Also of note is that the built-in sort in JavaScript mutates the original array while sortBy() returns a new array.

On one hand returning a new array is great because it doesn't mutate the old one. On the other hand, it's probably not a big deal here.

Hi @dmsnell,
I'm using the loadash sortBy which already handles exceptions.
In this specific case it protects the sorting even if the order hasn't set for the other elements in the categories array. The built-in sort doesn't handle that exception.

Antonio

dmsnell · 2017-10-05T19:48:53Z

Nice work @kuoko - I don't know how to evaluate the broader role this code would provide but I gave some comments on the code at a lower-level. Hope it's useful to you; please feel free to ping me if you have comments or questions about it!

dmsnell · 2017-10-10T16:12:44Z

@kuoko I don't know if you intended to remove the package-lock.json file but it seems like we need to get those changes out of this PR

kuoko · 2017-10-12T08:58:13Z

Hi @dmsnell,
removing the package-lock.json was my mistake. It's ok now.
thanks a lot for your useful advises. I'm planning to work on it in the next few days.

travislopes · 2017-12-13T18:33:50Z

@kuoko Need any help with this PR? I was looking for this exact functionality for Gutenberg earlier today.

kuoko · 2017-12-15T11:57:22Z

HI @travislopes,
I was really busy recently.
I think I can handle it in the next few days but I will let you know if I need help.

Thanks a lot,
Antonio

dmsnell · 2017-12-19T19:42:51Z

blocks/api/categories.js

+
+	categories.push( category );
+	return categories;
+}


oh typing…we either get it in a robust and helpful way or we manually force it in unreliable and costly ways…

(this is not a comment on the code submission, but just me noting a place where having static typing could save us so much effort and provide us so much help)

dmsnell · 2017-12-19T19:45:36Z

blocks/api/categories.js

+
+	const sortedCategories = sortBy( categories, sortProperty );
+	return sortedCategories;
+}


is this function even used? I didn't see any usage other than in the tests, plus, it's not really doing much

is there a reason we would prefer to call this over directly calling sortBy()?

I would imagine that this function probably isn't needed and we could strip it out

@dmsnell Yes. The function is even more useless considering that we are not able to mutate categories. That's the reason why it was a reassignable variable in one of my previous commit. I've tried to keep it simple but I agree with you it's better to have an isolated PR for this.
It needs more discussions.

dmsnell · 2017-12-19T19:55:43Z

@kuoko thanks for continuing to iterate on this! I have a few very broad questions at this point, as I had a little more chance to evaluate the PR holistically.

Although I appreciate the strict verification checks going in to all your functions, I'm questioning if they are appropriate right there right now. Do we find this same level of verbosity around the rest of the project? Are those checks protecting us against the kinds of errors we expect to see? Do they add a bunch of code while still allowing through the kinds of errors we want to prevent?
Have we explored different needs for adding categories? Do we need to be concerned with encouraging category explosion where everyone creates their own category? Do we need to guard against things that are currently manually crafted but becoming open, like category naming and length?
Is it right to introduce the ability to reorder the categories? How does this jive with the UI efforts and semantics? Should we figure out a proper ordering and introduce that in an isolated PR? Should blocks be able to set order vs. defining a kind of order priority?

Looking forward to the discussion! This is great stuff you are doing.

carlhancock · 2017-12-19T21:13:39Z

@dmsnell Just commenting on number 2 above. From a plugin standpoint categories for blocks would be extremely useful. I do not think it will be uncommon for plugins to have multiple blocks. Especially more feature rich plugins such as WooCommerce. They will likely have a variety of blocks for different uses.

Right now if WooCommerce created a suite of blocks where would they even be housed? Under Common? Format? Layout? Widgets? They would look better under a WooCommerce category grouped together.

Being able to extend the categories would greatly aid in discoverability.

…By to getSortedCategories - Use Lodash's _.find instead of _.findIndex

…, use some function instead of find, getSortedCategories doesn't need to be mutative

kuoko · 2018-01-19T14:02:27Z

I've simplified the module.
Not sure if we are now ready to merge or we prefer to discuss it more.

dmsnell

Seems reasonable what's here. I have mostly neutral feelings about it. Please let me know if there's some way I can be helpful. 😄

sc0ttkclark · 2018-01-19T21:31:41Z

Looks great! Not to distract from this PR which should definitely get merged, but I'm hoping there might be a consideration for a PHP API to register new categories in a future PR. Throwing my two cents out here, registering blocks and categories via PHP is going to be necessary for wide-scale adoption and a successful rollout.

youknowriad · 2018-02-02T14:35:03Z

With regards to the last extensibility work, it looks like the consensus is to avoid adding new register*APIs in favor of hooks and filters (cc @gziolo). Is it possible to update this PR to use a filter instead?

gziolo · 2018-02-05T12:44:15Z

We are exploring different ways to consolidate attributes definition and consider having a server-side awareness of block types as described in #2751. It is highly possible that verification for category names is going to be moved to PHP when we decide to take that route.

There are also a few question we need to answer before we proceed with those changes:

Should we use PHP based hook instead to expose new categories?
Should it be possible to blacklist blocks for the existing core category? Should we stop displaying embeds tab in the UI as proposed in If all embed blocks have been removed via wp.blocks.unregisterBlockType(), the Embeds tab should not appear in the inserter #3992 when they are blacklisted?
Should we use JS filter to achieve the same result as @youknowriad mentioned and commented inline here?

gziolo · 2018-02-05T12:54:48Z

blocks/api/categories.js

@@ -28,3 +38,48 @@ const categories = [
 export function getCategories() {
 	return categories;
 }
+


It's also possible to achieve a very similar result using this code:

import { applyFilters } from '@wordpress/hooks'; let cachedCategories; export function getCategories() { if ( ! cachedCategories ) { const extraCategories = ...applyFilters( 'blocks.getExtraCategories', [] ); // we can perform optonal validation here and filter out all items that don't have slug and name cachedCategories = [ ...extraCategories, ...categories ]; } return cachedCategories; }

Usage:

function addCategory( extraCategories ) { return [ ...extraCategories, { slug: 'my-category', title: __( 'My category' ) } ]; } wp.hooks.addFilter( 'blocks.getExtraCategories', 'my-plugin/extra-categories', addCategory );

also, since JavaScript functions are much cheaper than PHP functions we can wrap via composition and keep different logic separate 😄

import { once } from 'lodash' const getCategories = once( () => [ ...applyFilters( 'blocks.getExtraCategories', [] ), ...categories, ] )

and without lodash it's easy to recreate once

const once = f => { let isPrimed = false let value return ( ...args ) => ! isPrimed ? ( isPrimed = true, value = f( ...args ) ) : value }

@dmsnell thanks for making it even simpler :)

carlhancock · 2018-02-19T21:53:57Z

Any update on this @gziolo @youknowriad @kuoko?

We are already encountering the lack of being able to customize, add, and re-order categories within the block inserter as being an issue from a development standpoint.

We aren't planning a single block for Gutenberg. In order to adopt Gutenberg full bore and go all in with making our product Gutenberg friendly that is going to involve a suite of blocks. Being able to group those blocks together is going to be very important from a UX standpoint.

So this is a PR that we are particularly interested in and can't wait to see merged into Gutenberg core.

gziolo · 2018-02-22T10:24:37Z

We aren't planning a single block for Gutenberg. In order to adopt Gutenberg full bore and go all in with making our product Gutenberg friendly that is going to involve a suite of blocks. Being able to group those blocks together is going to be very important from a UX standpoint.

This is on the roadmap for sure. We want to tackle other issues first before handling this one. One of the examples which also fits your description is: Limit Block to Certain Post Type(s) . I don't know what use case you have in particular, but having the ability to pick which blocks are loaded for a given post type seems to be the most requested feature. We are looking how to extend Templates to support also this. This should help to decide how to tackle categories. I already shared my thought about categories in my previous comment: #1732 (comment). There are more things to take into consideration to have it implemented in a consistent way that covers the majority of cases all plugins might want.

carlhancock · 2018-02-23T14:55:37Z

@gziolo Thanks for the update. We are getting ready to release our second Gutenberg block publicly for testing and it's already clear we need a category to group our blocks so not only is the user experience better but it's also easier to document and support. So we are looking forward to this one allowing us to make block discovery much more user friendly.

mejta · 2018-06-21T16:50:03Z

@gziolo Hello, is there any progress with this feature? I have a custom blocks for my theme and I would like to distinguish them by category. I haven't found any way how to extend that so it would be great to have a filter for that.

youknowriad · 2018-06-21T18:45:41Z

The internal APIs to support this are already there, we're using a store to keep track of the categories. The store already supports an ADD_CATEGORY action. The last part to make this available is to add the wrapping action creator and function. This should be straightforward and hopefully we get this in for the next release or so.

gziolo · 2018-06-28T14:08:26Z

An alternative approach was proposed in #7606.

gziolo · 2018-06-28T14:09:01Z

Thanks for all work put into this PR, it was very helpful to come up with a final proposal. Let us know if that works.

Move DependencyGraph.js so the original file is not left over after `yarn bundle`

braders reviewed Jul 5, 2017

View reviewed changes

mtias added [Feature] Block API API that allows to express the block paradigm. [Type] Question Questions about the design or development of the editor. labels Jul 5, 2017

aduth reviewed Aug 3, 2017

View reviewed changes

kuoko force-pushed the update/block-categories branch 2 times, most recently from 6c0023f to 2497cd1 Compare August 7, 2017 13:56

aduth reviewed Aug 11, 2017

View reviewed changes

kuoko mentioned this pull request Sep 15, 2017

Extending Block Categories #2727

Closed

ahmadawais suggested changes Sep 15, 2017

View reviewed changes

dmsnell reviewed Oct 5, 2017

View reviewed changes

kuoko force-pushed the update/block-categories branch 2 times, most recently from d751a55 to 8dd16e6 Compare October 10, 2017 11:24

kuoko force-pushed the update/block-categories branch from 8dd16e6 to ed3567c Compare October 12, 2017 08:36

kuoko force-pushed the update/block-categories branch from 6d5442d to c78d533 Compare December 19, 2017 17:54

dmsnell reviewed Dec 19, 2017

View reviewed changes

kuoko added 6 commits January 18, 2018 17:55

Add more test-unit

c574c2a

Use full-lenght definition category (not cat) - Change sortCategories…

617eb05

…By to getSortedCategories - Use Lodash's _.find instead of _.findIndex

documentation and lingo tweaks

c8a9af4

Resolve conflict

a4d0e37

Make categories const, explicit categoryNamePattern regexp as costant…

775cf03

…, use some function instead of find, getSortedCategories doesn't need to be mutative

Remove all the methods used for sorting categories

02a365a

kuoko force-pushed the update/block-categories branch from 36a29c9 to 02a365a Compare January 18, 2018 17:58

Remove identifiers deleted from the categories module

9afca9d

dmsnell reviewed Jan 19, 2018

View reviewed changes

gziolo added the [Feature] Extensibility The ability to extend blocks or the editing experience label Jan 27, 2018

gziolo reviewed Feb 5, 2018

View reviewed changes

gziolo mentioned this pull request Feb 5, 2018

Block API: Server-side awareness of block types #2751

Closed

gziolo mentioned this pull request Feb 19, 2018

API for Inter-block communication #5012

Closed

aduth mentioned this pull request Apr 24, 2018

Validate or handle undefined block category #2264

Closed

gziolo mentioned this pull request Jun 28, 2018

Blocks: Add a way to manage block categories #7606

Merged

4 tasks

gziolo closed this Jun 28, 2018

Tug pushed a commit that referenced this pull request Jan 16, 2020

Merge pull request #1732 from wordpress-mobile/yarn_version_cleanup

9ba1ee9

Move DependencyGraph.js so the original file is not left over after `yarn bundle`

Add new function to register a new block category #1732

Add new function to register a new block category #1732

Conversation

kuoko commented Jul 5, 2017 • edited Loading

braders left a comment

Choose a reason for hiding this comment

kuoko commented Jul 6, 2017

kuoko commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 7, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmadawais left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuoko commented Sep 18, 2017

Choose a reason for hiding this comment

dmsnell Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell commented Oct 5, 2017

dmsnell commented Oct 10, 2017

kuoko commented Oct 12, 2017

travislopes commented Dec 13, 2017

kuoko commented Dec 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell commented Dec 19, 2017

carlhancock commented Dec 19, 2017 • edited Loading

kuoko commented Jan 19, 2018

dmsnell left a comment

Choose a reason for hiding this comment

sc0ttkclark commented Jan 19, 2018

youknowriad commented Feb 2, 2018

gziolo commented Feb 5, 2018 • edited Loading

gziolo Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlhancock commented Feb 19, 2018

gziolo commented Feb 22, 2018

carlhancock commented Feb 23, 2018

mejta commented Jun 21, 2018

youknowriad commented Jun 21, 2018 • edited Loading

gziolo commented Jun 28, 2018

gziolo commented Jun 28, 2018

kuoko commented Jul 5, 2017 •

edited

Loading

codecov bot commented Aug 7, 2017 •

edited

Loading

dmsnell Oct 5, 2017 •

edited

Loading

dmsnell Oct 5, 2017 •

edited

Loading

carlhancock commented Dec 19, 2017 •

edited

Loading

gziolo commented Feb 5, 2018 •

edited

Loading

gziolo Feb 5, 2018 •

edited

Loading

youknowriad commented Jun 21, 2018 •

edited

Loading