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 block collections #17609

Merged
merged 4 commits into from Jan 7, 2020
Merged

Conversation

@scruffian
Copy link
Contributor

scruffian commented Sep 26, 2019

Description

As proposed in #16866, this adds a new function registerBlockCollection( namespace, title, icon = {} );

This enables blocks to be added to both collections and categories, ensuring that blocks can be put in the category that they make the most sense in, but also make it possible to see all the blocks coming from a single source.

namespace is matched against a block prefix.
title shows in the block inserter.
icon will show alongside the title, if defined

How has this been tested?

Tested in Chrome on a .org installation with no other plugins.

Screenshots

Screenshot 2019-09-26 at 14 36 10

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
Copy link
Member

gziolo left a comment

This is definitely a good direction. It raises some questions like:

  • what do we about all currently registered custom categories?
  • do we still want to allow to register custom categories?
  • how do we encourage plugin developers to use collections instead of categories?

We still need documentation, a note in the changelog and unit tests for new APIs.

* @param {Object} tite The title to show in the inserter
* @param {Object} icon The icon to show in the inserter
*
* @return {?WPBlock} The block, if it has been successfully registered;

This comment has been minimized.

Copy link
@gziolo

gziolo Sep 26, 2019

Member

The description for the return value needs to be updated.

@@ -30,6 +30,7 @@ import {
} from '@wordpress/components';
import {
getCategories,
getCollections,

This comment has been minimized.

Copy link
@gziolo

gziolo Sep 26, 2019

Member

Wouldn't it be more reliable to get both collections and categories through withSelect HOC?

@@ -195,6 +207,10 @@ _Returns_

<!-- START TOKEN(Autogenerated actions) -->

<a name="addBlockCollection" href="#addBlockCollection">#</a> **addBlockCollection**

Undocumented declaration.

This comment has been minimized.

Copy link
@gziolo

gziolo Sep 26, 2019

Member

Documentation is missing here.

This comment has been minimized.

Copy link
@scruffian

scruffian Sep 27, 2019

Author Contributor

I added it, I guess this will update automatically?

This comment has been minimized.

Copy link
@gziolo

gziolo Oct 7, 2019

Member

npm run docs:build should update this document.

@scruffian

This comment has been minimized.

Copy link
Contributor Author

scruffian commented Sep 26, 2019

Thanks for the review @gziolo. Some thoughts:

do we still want to allow to register custom categories?
what do we about all currently registered custom categories?

I think there is still value in the custom categories. The way I see it this just adds another option for block developers, but they can continue doing it how they are without a problem.

how do we encourage plugin developers to use collections instead of categories?

I'll add it to the docs.

@scruffian

This comment has been minimized.

Copy link
Contributor Author

scruffian commented Sep 26, 2019

Another question this raises is, should we should blocks in both sections when you search, for example:

Screenshot 2019-09-26 at 17 18 59

@scruffian

This comment has been minimized.

Copy link
Contributor Author

scruffian commented Sep 27, 2019

@gziolo I added a test. Is that sufficient?

@@ -1,4 +1,4 @@
/* eslint no-console: [ 'error', { allow: [ 'error' ] } ] */
/* eslint no-8: [ 'error', { allow: [ 'error' ] } ] */

This comment has been minimized.

Copy link
@gziolo

gziolo Oct 7, 2019

Member

It doesn't look like an expected change.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Oct 7, 2019

@youknowriad and @mtias - can you share your feedback on this PR?

Copy link
Contributor

youknowriad left a comment

Left a small comment but I like the addition 👍

*/
export function getCollections() {
return select( 'core/blocks' ).getCollections();
}

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 7, 2019

Contributor

Is this necessary? Can't we rely on selectors instead? (Selectors are reactive and not global functions).
I know we do the same mistake for other things like "categories", "block types"... but ideally, we don't continue the trend for new APIs.

This comment has been minimized.

Copy link
@scruffian

scruffian Oct 11, 2019

Author Contributor

Happy to make the change, but I'm not sure what it should be. I just copied the other (bad) examples!

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 11, 2019

Contributor

The idea is that we don't really need this function.

components can call select( 'core/blocks' ).getCollections() directly in withSelect or useSelect

This comment has been minimized.

Copy link
@scruffian

scruffian Dec 4, 2019

Author Contributor

Done!

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Oct 7, 2019

One more question, how this would work for blocks registered on the server? Do you think this should be something which should be handled only on the front-end?

registerBlockCollection( namespace, title, icon = {} );

One more thing, as far as I remember, all the APIs that register something take an object as 2nd param. Should we follow the same pattern here? In general, it's more flexible as it allows to easily introduce more options in the future.

* @param {Object} icon The icon to show in the inserter
*
*/
export function registerBlockCollection( namespace, title, icon ) {

This comment has been minimized.

Copy link
@gziolo

gziolo Oct 25, 2019

Member

All the existing public API methods which register something use settings object as 2nd param, it would be great to align, see:

export function registerFormatType( name, settings ) {

export function registerPlugin( name, settings ) {

export function registerBlockType( name, settings ) {

All of them offer unregister* function as well which returns the registered object for more control for 3rd party plugings.

This comment has been minimized.

Copy link
@scruffian

scruffian Dec 4, 2019

Author Contributor

Done!

@scruffian

This comment has been minimized.

Copy link
Contributor Author

scruffian commented Dec 4, 2019

One more question, how this would work for blocks registered on the server? Do you think this should be something which should be handled only on the front-end?

I think having it front-end only is fine for now. We can add a server side option for it if there's a demand.

{ slug: 'reusable', title: 'Reusable Blocks', icon: null },
];

const collections = {

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 6, 2019

Member

What's the reason that collections use a different structure than categories?

In general, it seems to work fundamentally different.

This comment has been minimized.

Copy link
@scruffian

scruffian Dec 6, 2019

Author Contributor

The way we access collections makes it easier to do so using an object than an array, so this makes the code simpler. I can change it to be an array if you think that matching the structure for categories is better.

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 6, 2019

Member

I'm more concerned about the reducer and type of actions used. I don't remember how categories are managed by plugins. I would need to find some snippets to do the comparison. Ideally, we provide a way to manage it in a similar way on a higher level.

This comment has been minimized.

@scruffian scruffian force-pushed the scruffian:add/block-collections branch from a84ebe0 to 57c376f Dec 6, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 19, 2019

What's blocking here? Can we try to refresh it and land it?

@scruffian scruffian force-pushed the scruffian:add/block-collections branch from 57c376f to 45bbf03 Dec 19, 2019
* Registers a new block collection to group blocks in the same namespace in the inserter.
*
* @param {string} namespace The namespace to group blocks by in the inserter; corresponds to the block namespace
* @param {Object} title, icon The title to show in the inserter, The icon to show in the inserter

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 19, 2019

Contributor

Would be cool to try to type these like suggested by @aduth in #18838


const matchingCategories = element.querySelectorAll(
'.components-panel__body-toggle'
);

<<<<<<< HEAD

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 19, 2019

Contributor

conflict issue

@scruffian scruffian force-pushed the scruffian:add/block-collections branch from d4df845 to 7a1da8d Dec 19, 2019
@scruffian

This comment has been minimized.

Copy link
Contributor Author

scruffian commented Dec 19, 2019

@youknowriad I've rebased this and fixed all the conflicts. Thanks for looking.

Copy link
Contributor

draganescu left a comment

The collection appears as documented, the test collection still needs to be removed.

@@ -146,6 +148,12 @@ export const registerCoreBlocks = () => {
video,
].forEach( registerBlock );

// This line is just for testing
registerBlockCollection( 'core', {
title: 'Core',

This comment has been minimized.

Copy link
@draganescu

draganescu Dec 20, 2019

Contributor

As this seems ready to land, this test collection should be removed before we merge

@scruffian

This comment has been minimized.

Copy link
Contributor Author

scruffian commented Dec 20, 2019

@draganescu thanks, done :)


Blocks can be added to collections, grouping together all blocks from the same origin

`registerBlockCollection` takes three parameters `namespace`, `title` and `icon`.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 23, 2019

Contributor

I think the wording is incorrect since it's in fact two arguments.

Copy link
Contributor

youknowriad left a comment

Thanks for your work here, this is looking good. Ideally, we add an e2e test (plugin test) to validate this API. It could also be done as a follow-up.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 23, 2019

(Let me know when it's rebased... so I can merge)

* Registers a new block collection to group blocks in the same namespace in the inserter.
*
* @param {string} namespace The namespace to group blocks by in the inserter; corresponds to the block namespace
* @param {Object} title, icon The title to show in the inserter, The icon to show in the inserter

This comment has been minimized.

Copy link
@aduth

aduth Jan 2, 2020

Member

This comma separation is not a valid syntax for JSDoc.

*
* @param {Object} state Data state.
*
* @return {Array} Collections list.

This comment has been minimized.

Copy link
@aduth

aduth Jan 2, 2020

Member

Based on the state shape, I don't think this returns an array. Should it be returning an array though? That seems like what I might expect to receive as a consumer.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 6, 2020

Would you mind a rebase here so we can consider merging it?

@scruffian scruffian force-pushed the scruffian:add/block-collections branch from 08c388c to b3a7162 Jan 6, 2020
@scruffian

This comment has been minimized.

Copy link
Contributor Author

scruffian commented Jan 6, 2020

@youknowriad I rebased this and fixed the incorrect docs. Thanks!

@youknowriad youknowriad merged commit 0cb146d into WordPress:master Jan 7, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 7, 2020

Thanks for your work here @scruffian

@scruffian scruffian deleted the scruffian:add/block-collections branch Jan 7, 2020
@ellatrix ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020
@jorgefilipecosta jorgefilipecosta mentioned this pull request Feb 12, 2020
18 of 25 tasks complete
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.