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

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

Merged
merged 4 commits into from
Jul 25, 2017

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jul 3, 2017

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.

@youknowriad youknowriad added the [Feature] Inserter The main way to insert blocks using the + button in the editing interface label Jul 3, 2017
@youknowriad youknowriad self-assigned this Jul 3, 2017
@swissspidy
Copy link
Member

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

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

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

Should we add this useOnce property to the more block?

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

@westonruter
Copy link
Member

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 westonruter mentioned this pull request Jul 4, 2017
@westonruter
Copy link
Member

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

@youknowriad
Copy link
Contributor Author

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

@BE-Webdesign
Copy link
Contributor

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

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

@youknowriad
Copy link
Contributor Author

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

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.

@westonruter westonruter mentioned this pull request Jul 8, 2017
@swissspidy
Copy link
Member

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

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 aduth self-requested a review July 18, 2017 21:16
}

// Return the name of the next block type.
return list[ nextIndex ].name;
const block = blockTypes[ nextIndex ];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@youknowriad
Copy link
Contributor Author

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

@aduth
Copy link
Member

aduth commented Jul 24, 2017

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.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

stuck

@youknowriad
Copy link
Contributor Author

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.

return list[ nextIndex ].name;
const blockType = blockTypes[ nextIndex ];
if ( this.isDisabledBlock( blockType ) ) {
return this.findByIncrement( blockTypes, increment > 0 ? increment + 1 : increment - 1 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

BE-Webdesign commented Jul 24, 2017

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

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

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

@youknowriad youknowriad merged commit b935bbf into master Jul 25, 2017
@youknowriad youknowriad deleted the add/use-block-once branch July 25, 2017 11:31
@aaronjorbin
Copy link
Member

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

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

aduth commented Jul 25, 2017

The inserter menu is a good candidate for tests though.

ceyhun pushed a commit that referenced this pull request Apr 22, 2020
* update ref to master (Columns Block)
ceyhun pushed a commit that referenced this pull request Apr 22, 2020
* Release v1.26.0 (#2153)

* Add tests for Latest-Posts Bock

* Have the Automation tests Scroll the Block window to help locate Block buttons on Android

* Update gutenberg reference

* Update Gutenberg ref

* Update Gutenberg ref

* New template for release PRs

This PR will add a new template for release PRs to make it easier to check all the steps needed and to standardize the release PRs.

This template can then be used by using this link: `https://github.com/wordpress-mobile/gutenberg-mobile/pull/new?template=release_pull_request.md`

* Update template file.

* Fix: remove extra padding for post title and first `Paragraph` block (#2095)

* Fix: remove extra padding for post title and first `Paragraph` block

* Update Gutenberg ref

* Add a new androidReplacements function to comply with Android Typography lint rules

* Make sure the file gutenberg.pot exists before generating android and ios strings.

* Update Gutenberg ref

* Update gutenberg ref

* Update gutenberg ref

* Update gutenberg reference

* Gutenberg update

* Update Gutenberg ref

* Update Gutenberg ref

* Update Gutenberg ref

* Update Gutenberg ref

* Fix: prevent ripple effect on slider cell in BottomSheet and disable it completely on iOS (#2023)

* prevent ripple effect on slider cell in BottomSheet and disable it completely on iOS

* Update Gutenberg ref

* Update Gutenberg ref

* Accept multiple headers through OnAuthHeaderRequestedListener (#2080)

* Blog layout template (#2114)

* Update Gutenberg ref

* Update Gutenberg ref

* Update gutenberg reference

* Fix failing UI tests

Try scrolling in the Inserter for all platforms

* Disable the failing test on iOS

Co-authored-by: Matthew Kevins <mmkevins@yahoo.com>
Co-authored-by: Pinar Olguc <pinarolguc@gmail.com>

* Update gutenberg reference

* Feat: Column block (#1661)

* update ref to master (Columns Block)

* Update gutenberg reference

* Fix Latests Posts Tests by expanding the scroll to button functionality

* Fix lint issue

* Fix typography breakage in master

To a version where the typography panel is not added to block settings.

* Update GB reference.

* Correct slider step value (#2119)

* Update ref: Correct slider step accordingly to the platform

* Update gb ref

Co-authored-by: Pinar Olguc <pinarolguc@gmail.com>

* v1.26.0

* Add some missing release notes

* Update Podfile.lock

* Update gb ref

* Update bundles

Co-authored-by: Chip Snyder <chip.snyder3@gmail.com>
Co-authored-by: Matthew Kevins <mmkevins@yahoo.com>
Co-authored-by: Gerardo Pacheco <gerardo.pacheco@automattic.com>
Co-authored-by: Sérgio Estêvão <sergioestevao@gmail.com>
Co-authored-by: jbinda <jakub.binda@gmail.com>
Co-authored-by: Chip <chip.snyder@automattic.com>
Co-authored-by: Maxime Biais <maxime.biais@gmail.com>
Co-authored-by: Tugdual de Kerviler <dekervit@gmail.com>
Co-authored-by: Klymentiy Haykov <forsver@gmail.com>
Co-authored-by: Matthew Kevins <mkevins@users.noreply.github.com>
Co-authored-by: Luke Walczak <lukasz.walczak.pwr@gmail.com>

* Update gb ref

Co-authored-by: Chip Snyder <chip.snyder3@gmail.com>
Co-authored-by: Matthew Kevins <mmkevins@yahoo.com>
Co-authored-by: Gerardo Pacheco <gerardo.pacheco@automattic.com>
Co-authored-by: Sérgio Estêvão <sergioestevao@gmail.com>
Co-authored-by: jbinda <jakub.binda@gmail.com>
Co-authored-by: Chip <chip.snyder@automattic.com>
Co-authored-by: Maxime Biais <maxime.biais@gmail.com>
Co-authored-by: Tugdual de Kerviler <dekervit@gmail.com>
Co-authored-by: Klymentiy Haykov <forsver@gmail.com>
Co-authored-by: Matthew Kevins <mkevins@users.noreply.github.com>
Co-authored-by: Luke Walczak <lukasz.walczak.pwr@gmail.com>
Tug added a commit that referenced this pull request May 12, 2020
* Add tests for Latest-Posts Bock

* Have the Automation tests Scroll the Block window to help locate Block buttons on Android

* Update gutenberg reference

* Update Gutenberg ref

* Update Gutenberg ref

* New template for release PRs

This PR will add a new template for release PRs to make it easier to check all the steps needed and to standardize the release PRs.

This template can then be used by using this link: `https://github.com/wordpress-mobile/gutenberg-mobile/pull/new?template=release_pull_request.md`

* Update template file.

* Fix: remove extra padding for post title and first `Paragraph` block (#2095)

* Fix: remove extra padding for post title and first `Paragraph` block

* Update Gutenberg ref

* Add a new androidReplacements function to comply with Android Typography lint rules

* Make sure the file gutenberg.pot exists before generating android and ios strings.

* Update Gutenberg ref

* Update gutenberg ref

* Update gutenberg ref

* Update gutenberg reference

* Gutenberg update

* Update Gutenberg ref

* Update Gutenberg ref

* Update Gutenberg ref

* Update Gutenberg ref

* Fix: prevent ripple effect on slider cell in BottomSheet and disable it completely on iOS (#2023)

* prevent ripple effect on slider cell in BottomSheet and disable it completely on iOS

* Update Gutenberg ref

* Update Gutenberg ref

* Accept multiple headers through OnAuthHeaderRequestedListener (#2080)

* Blog layout template (#2114)

* Update Gutenberg ref

* Update Gutenberg ref

* Update gutenberg reference

* Fix failing UI tests

Try scrolling in the Inserter for all platforms

* Disable the failing test on iOS

Co-authored-by: Matthew Kevins <mmkevins@yahoo.com>
Co-authored-by: Pinar Olguc <pinarolguc@gmail.com>

* Update gutenberg reference

* Feat: Column block (#1661)

* update ref to master (Columns Block)

* Update gutenberg reference

* Fix Latests Posts Tests by expanding the scroll to button functionality

* Fix lint issue

* Fix typography breakage in master

To a version where the typography panel is not added to block settings.

* Update GB reference.

* Correct slider step value (#2119)

* Update ref: Correct slider step accordingly to the platform

* Update gb ref

Co-authored-by: Pinar Olguc <pinarolguc@gmail.com>

* v1.26.0

* Add some missing release notes

* Update Podfile.lock

* Update gb ref

* Update bundles

Co-authored-by: Chip Snyder <chip.snyder3@gmail.com>
Co-authored-by: Matthew Kevins <mmkevins@yahoo.com>
Co-authored-by: Gerardo Pacheco <gerardo.pacheco@automattic.com>
Co-authored-by: Sérgio Estêvão <sergioestevao@gmail.com>
Co-authored-by: jbinda <jakub.binda@gmail.com>
Co-authored-by: Chip <chip.snyder@automattic.com>
Co-authored-by: Maxime Biais <maxime.biais@gmail.com>
Co-authored-by: Tugdual de Kerviler <dekervit@gmail.com>
Co-authored-by: Klymentiy Haykov <forsver@gmail.com>
Co-authored-by: Matthew Kevins <mkevins@users.noreply.github.com>
Co-authored-by: Luke Walczak <lukasz.walczak.pwr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blocks API: define a way for blocks to specify they can occur only once
7 participants