Block API: Adding a useOnce property in the block settings to forbid using it multiple times

[youknowriad]

closes #1465

While doing this, I noticed some errors while navigating using arrows in the inserter, it should be fixed now. (The warning has been reported in #1606)

Testing instructions

• We don't have any block like that for now, you'll have to add useOnce: true to one of the current block settings to try it.

[swissspidy]

One comment regarding such a feature was:

On that note, such a feature would be nice in general for the API so third-party plugins could introduce blocks that have a specific limit to the amount of times it can be added to the editor.

This could perhaps be achieved by using _.countBy instead of _.find.

[youknowriad]

@swissspidy Yes, I saw that comment but before introducing this feature, I was wondering if it's really worth it. I can't seem to find any use-case for it. The code change is small but I find useOne property to be descriptive more than a potential useCount

[BE-Webdesign]

I find useOne property to be descriptive more than a potential useCount

Agreed, I like the approach taken here.

Should we add this useOnce property to the more block?

[swissspidy]

Should we add this useOnce property to the more block?

That's the plan and where this idea originated from, so yeah.

[westonruter]

What about, say, a page template that defines that it allows 3 image blocks. I suppose that constraint would have to be applied at a different level?

[westonruter]

Impending use case for useOnce is the More block in #983.

[youknowriad]

Should we merge this as is? The more block is not merged yet and could be rebased.

[BE-Webdesign]

I think waiting for the more block is good (didn't realize it wasn't in), as that is the use case this is going to make work.

I also wonder if this should be a block attribute though and not something solely set by the editor/inserter.

[swissspidy]

The more block is pretty much good to go, so …

[youknowriad]

I also wonder if this should be a block attribute though and not something solely set by the editor/inserter.

Not sure I understand, it's already a block setting?

[BE-Webdesign]

Not sure I understand, it's already a block setting?

From my reading of the code, when the more block is registered it will add a useOnce: true property to the block settings. ( I used the word attribute instead of setting ). I wonder if this really should be a setting for the block, because why does it really need to know that it is used once. I fear that we may end up just adding lots of settings to blocks to solve various use cases. The inserter, instead, could reference a map of blocks that shows how many times they can be used.

Just throwing out an idea. I am not opposed to this PR in anyway, as it should work well. I am also not 100% sure that we are adding the useOnce as a setting on the block, that is just how I read the code.

[swissspidy]

From my reading of the code, when the more block is registered it will add a useOnce: true property to the block settings. ( I used the word attribute instead of setting ).

You wont see useOnce: true in the serialized post_content (e.g. no <!-- wp:core/more {"useOnce": true} -->) , if that's what you mean.

[BE-Webdesign]

You wont see useOnce: true in the serialized post_content (e.g. no ) , if that's what you mean.

Nope, I mean more that it doesn't really belong to the blockType as a property. If we continue to keep adding properties to block types to solve various use cases, we will end up bloating the block types.

Not opposed to this approach at all, just proposing another way we could approach this problem, by having the block counts be purely handled in the inserter.

[aduth]

Can we call this variable blockType to avoid confusion? Also the argument and name of isDisabledBlockType.

[youknowriad]

Back from vacations :). Rebased here, it should be ok.

[aduth]

I fear that we may end up just adding lots of settings to blocks to solve various use cases. The inserter, instead, could reference a map of blocks that shows how many times they can be used.

Where would said mapping exist? How do plugin authors opt-in to include their block type in the set?

I'm sympathetic to the concern of over-bloating the block settings API, but this seems to me a characteristic of a block type nonetheless.

[aduth]

I think we ought to account for block disabling in showing the insertion point.

• It doesn't make sense that an insertion point be shown when hovering a disable block
• It becomes stuck if you hover over and then away from the disabled block type option (unless moving to a different, non-disabled block type)

[youknowriad]

Good catch @aduth. It should be better now.

Also, I explicitly kept this block setting undocumented (to avoid over-bloating the block settings API), This may be useless for block authors. We could still document it later if needed.

[aduth]

In this case I might have opted for a simple loop rather than recursion, because the findIndex above is non-trivial.

diff --git a/editor/inserter/menu.js b/editor/inserter/menu.js
index 33680f4b..4b95bb9a 100644
--- a/editor/inserter/menu.js
+++ b/editor/inserter/menu.js
@@ -139,10 +139,21 @@ class InserterMenu extends Component {
 
        findByIncrement( blockTypes, increment = 1 ) {
                const currentIndex = findIndex( blockTypes, ( blockType ) => this.state.currentFocus === blockType.name );
-               const nextIndex = currentIndex + increment;
                const highestIndex = blockTypes.length - 1;
                const lowestIndex = 0;
 
+               let nextIndex = currentIndex;
+               let blockType;
+               do {
+                       nextIndex += increment;
+
+                       // Return the name of the next block type.
+                       blockType = blockTypes[ nextIndex ];
+                       if ( blockType && ! this.isDisabledBlock( blockType ) ) {
+                               return blockType.name;
+                       }
+               } while ( blockType );
+
                if ( nextIndex > highestIndex ) {
                        return 'search';
                }
@@ -150,14 +161,6 @@ class InserterMenu extends Component {
                if ( nextIndex < lowestIndex ) {
                        return 'search';
                }
-
-               // Return the name of the next block type.
-               const blockType = blockTypes[ nextIndex ];
-               if ( this.isDisabledBlock( blockType ) ) {
-                       return this.findByIncrement( blockTypes, increment > 0 ? increment + 1 : increment - 1 );
-               }
-
-               return blockType.name;
        }
 
        findNext( blockTypes ) {

[BE-Webdesign]

Where would said mapping exist? How do plugin authors opt-in to include their block type in the set?

The mapping would exist for where it related to. So in this instance it would exist on the inserter. We could have an inserter property called usableBlockTypes (terrible name, can't think of anything better at the moment).

usableBlockTypes = {
  'wp-core/more': {
    allowedInstances: 1,
  }
}

Then use hooks.applyFilters() for extending.

I'm sympathetic to the concern of over-bloating the block settings API, but this seems to me a characteristic of a block type nonetheless.

Yup, I can agree with that. I was throwing the idea out to see if it caught on. I think that in different contexts blockTypes might have different numbers that can be used. For instance a page layout, that should describe what and how many blockTypes can be used. The blockType itself having a property in that instance wouldn't make sense, because it will need to be different for different situations, which would lead to messy conditionals.

[youknowriad]

usableBlockTypes = {
'wp-core/more': {
allowedInstances: 1,
}
}

This makes the inserter aware of the existence of such blocks. I think avoiding hardcoding things like that is a good thing (we're even avoiding hard-coding the default block and the unknown block). What if the more block is removed, renamed or unregisted.

I think that in different contexts blockTypes might have different numbers that can be used. For instance a page layout, that should describe what and how many blockTypes can be used.

Good point, but this is probably something different (another API or a persisted settings somewhere).

[youknowriad]

I'm going to merge this, of course, it's fine to revisit but this will help us move the more block forward.

[aaronjorbin]

It looks like this didn't update any of the documentation or include any tests to ensure that the API works as expected. It would be good to make sure this isn't a hidden feature.

[youknowriad]

The lack of documentation here is intentional. We do not want this to be a public API yet (#1661 (comment)) but more for internal usage for now.

[aduth]

The inserter menu is a good candidate for tests though.