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 new function to register a new block category #1732

Closed
wants to merge 17 commits into from

Conversation

@kuoko
Copy link
Contributor

commented Jul 5, 2017

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
Copy link

left a comment

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2017

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

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

This comment has been minimized.

Copy link
@aduth

aduth Aug 3, 2017

Member

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

This comment has been minimized.

Copy link
@kuoko

kuoko Aug 8, 2017

Author Contributor

Done.

Cheers, Antonio

@kuoko kuoko force-pushed the kuoko:update/block-categories branch 2 times, most recently from 6c0023f to 2497cd1 Aug 4, 2017

@codecov

This comment has been minimized.

Copy link

commented Aug 7, 2017

Codecov Report

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

Impacted file tree graph

@@            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.

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

This comment has been minimized.

Copy link
@aduth

aduth Aug 11, 2017

Member

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 )

This comment has been minimized.

Copy link
@aduth

aduth Aug 11, 2017

Member

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

This comment has been minimized.

Copy link
@kuoko

kuoko Sep 11, 2017

Author Contributor

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

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

This comment has been minimized.

Copy link
@aduth

aduth Aug 11, 2017

Member

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 😸

This comment has been minimized.

Copy link
@kuoko

kuoko Sep 11, 2017

Author Contributor

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. :)

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

This comment has been minimized.

Copy link
@aduth

aduth Aug 11, 2017

Member

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.

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

This comment has been minimized.

Copy link
@aduth

aduth Aug 11, 2017

Member

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

This comment has been minimized.

Copy link
@kuoko

kuoko Sep 11, 2017

Author Contributor

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

@kuoko kuoko referenced this pull request Sep 15, 2017
@ahmadawais
Copy link
Contributor

left a comment

A few documentation and lingo issues.

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

This comment has been minimized.

Copy link
@ahmadawais

ahmadawais Sep 15, 2017

Contributor

Consistent Documentation: Remove this empty line.

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

This comment has been minimized.

Copy link
@ahmadawais

ahmadawais Sep 15, 2017

Contributor

Consistent Documentation: Remove this empty line.

@@ -27,3 +30,105 @@ const categories = [
export function getCategories() {
return categories;
}

/**
* Register a new block category

This comment has been minimized.

Copy link
@ahmadawais

ahmadawais Sep 15, 2017

Contributor

Consistent Documentation: Add a period at the end.

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

This comment has been minimized.

Copy link
@ahmadawais

ahmadawais Sep 15, 2017

Contributor

'The block Category must be defined'

Having

  • b lowercase for block seems right.
  • C upercase for Category seems right.

Maybe change that everywhere?

@kuoko

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2017

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

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

This comment has been minimized.

Copy link
@dmsnell

dmsnell Oct 5, 2017

Contributor

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

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

This comment has been minimized.

Copy link
@dmsnell

dmsnell Oct 5, 2017

Contributor

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])+$/
);
return;
}
if ( categories.find( x => x.slug === category.slug ) ) {

This comment has been minimized.

Copy link
@dmsnell

dmsnell Oct 5, 2017

Contributor

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?

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

This comment has been minimized.

Copy link
@dmsnell

dmsnell Oct 5, 2017

Contributor

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.

This comment has been minimized.

Copy link
@kuoko

kuoko Dec 19, 2017

Author Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2017

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!

@kuoko kuoko force-pushed the kuoko:update/block-categories branch 2 times, most recently from d751a55 to 8dd16e6 Oct 10, 2017

@dmsnell

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2017

@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 kuoko force-pushed the kuoko:update/block-categories branch from 8dd16e6 to ed3567c Oct 12, 2017

@kuoko

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

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

@kuoko

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2017

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

@kuoko kuoko force-pushed the kuoko:update/block-categories branch from 6d5442d to c78d533 Dec 19, 2017


categories.push( category );
return categories;
}

This comment has been minimized.

Copy link
@dmsnell

dmsnell Dec 19, 2017

Contributor

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)


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

This comment has been minimized.

Copy link
@dmsnell

dmsnell Dec 19, 2017

Contributor

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

This comment has been minimized.

Copy link
@kuoko

kuoko Dec 23, 2017

Author Contributor

@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

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2017

@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.

  1. 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?

  2. 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?

  3. 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

This comment has been minimized.

Copy link

commented Dec 19, 2017

@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.

@kuoko kuoko force-pushed the kuoko:update/block-categories branch from 36a29c9 to 02a365a Jan 18, 2018

@kuoko

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2018

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

@dmsnell
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link

commented Jan 19, 2018

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.

@gziolo gziolo added this to In progress in Extensibility Jan 31, 2018

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 5, 2018

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 #3992 when they are blacklisted?
  • Should we use JS filter to achieve the same result as @youknowriad mentioned and commented inline here?
@@ -28,3 +38,48 @@ const categories = [
export function getCategories() {
return categories;
}

This comment has been minimized.

Copy link
@gziolo

gziolo Feb 5, 2018

Member

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 );

This comment has been minimized.

Copy link
@dmsnell

dmsnell Feb 5, 2018

Contributor

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
}

This comment has been minimized.

Copy link
@gziolo

gziolo Feb 5, 2018

Member

@dmsnell thanks for making it even simpler :)

@carlhancock

This comment has been minimized.

Copy link

commented Feb 19, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 22, 2018

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

This comment has been minimized.

Copy link

commented Feb 23, 2018

@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

This comment has been minimized.

Copy link

commented Jun 21, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2018

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 gziolo referenced this pull request Jun 28, 2018
4 of 4 tasks complete
@gziolo

This comment has been minimized.

Copy link
Member

commented Jun 28, 2018

An alternative approach was proposed in #7606.

@gziolo gziolo closed this Jun 28, 2018

Extensibility automation moved this from In progress to Done Jun 28, 2018

@gziolo

This comment has been minimized.

Copy link
Member

commented Jun 28, 2018

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.

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