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

Allow the specification of blocks allowed within columns #25778

Closed
wants to merge 21 commits into from

Conversation

JesserH
Copy link
Contributor

@JesserH JesserH commented Oct 1, 2020

Description

This an updated version of #25183.
I and @aristath developed a solution to the issue where an individual column could not define any specifically allowed blocks to be added in it. This also applies to newly added columns in the columns block (from the slider at the columns settings) also specify the blocks allowed to be added into that column. We believe that this is important to custom Gutenberg block development to nest blocks in a good and effective way.

How has this been tested?

Me and @aristath have tested this locally.

Screenshots

Screenshot 2020-09-09 at 12 14 13

Types of changes

We have added a new attribute called allowedBlocks in the column, and a childAllowedBlocks attribute within the columns block, which allows for defining the allowed blocks per individual column.

Example of use:

const allowedBlocks = [
  "core/paragraph",
  "core/separator",
  "core/cover",
];

//the code below can be used in a template
[ [ 'core/columns', { allowedChildBlocks: allowedBlocks }, [
      [ 'core/column', { allowedBlocks }, [
          //here a block is added, but with the allowedBlocks property more specifically allowed blocks can be added
          [ 'cgb/recipe-name-block', { name: attributes.name, allowedBlocks },  ],
      ] ],
      [ 'core/column', { allowedBlocks }, [
        [ 'cgb/recipe-description-block', { recipeDescription: attributes.description, allowedBlocks },  ],          
     ] ],
  ] ] ],

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

fixes #18161

@ajitbohra
Copy link
Member

@JesserH this seems to be a duplicate of #25183 which is already merged.

@ajitbohra ajitbohra closed this Oct 1, 2020
@aristath
Copy link
Member

aristath commented Oct 2, 2020

Reopening this one.
@ajitbohra #25183 appears merged in the Github UI but was not actually merged. There was a git mix-up and a force-push on the master branch just seconds after that PR got merged by mistake. As a result, the PR is marked as merged (even though it wasn't merged) and there is no way to reopen it.

@aristath aristath reopened this Oct 2, 2020
@aristath
Copy link
Member

aristath commented Oct 2, 2020

Tested and confirmed this works, details on #25183 (comment)

@raphaelparent
Copy link

Hi! I was wondering if there was any progress on this? Or if it was merged in part of another PR?

The last commits is dated of October 23rd, it was confirmed as working but the merge is blocked because of failed checks. I've looked around quit a bit and this is the most recent and up to date PR I've seen concerning the issue of allowed blocks in column elements.

@aristath
Copy link
Member

aristath commented Dec 1, 2020

@JesserH could you please sync your fork with the main repo? I believe most of the failing tests have been fixed since then

@aristath
Copy link
Member

aristath commented Dec 8, 2020

cc @talldan @ajitbohra

@talldan
Copy link
Contributor

talldan commented Dec 9, 2020

I was wondering if it'd be possible to use block context to just declare the attribute once on the parent columns block:
https://developer.wordpress.org/block-editor/developers/block-api/block-context/

The individual column could consume that context and pass it to the inner blocks of the column.

This way there would be a single source of truth for the allowedBlocks rather than duplicated attributes and there would then be less risk of them becoming out of sync.

@talldan
Copy link
Contributor

talldan commented Dec 9, 2020

Though I've maybe changed my mind looking at how the templateLock attribute was defined here - #26128

It seems like that PR specifically went for a solution where columns could have different template lock settings. It didn't really tackle whether users could modify the column count after the fact.

Given the main use-case for both would be templating, I wonder if it's worth matching that implementation for consistency as a first step, and then considering how it might work in the columns block as a separate change. What do you think?

@JesserH
Copy link
Contributor Author

JesserH commented Dec 9, 2020

@talldan I chose to implement the allowedChildBlocks for the blocks individually because of the columns block which has a slider. To my understanding it's not ideal to pass the value to the new block when the slider is moved upwards (so new blocks spawned).
Additionally, since it is used mostly for templating I think it is valuable to give the creator the customisability over his pre-defined columns, which is only improved functionality if you ask me. 😄 I'd personally prefer to keep the code as it is, but understand if you do want a refactor.
What is your view on this? 😃

@raphaelparent
Copy link

raphaelparent commented Jan 5, 2021

@aristath thanks for the follow up on this feature!

It seems like the merge is only awaiting your validation for the requested changes. Once it's merged and part of an update (not sure of the flow here) I'll be able to test it and give feedback if I find some problems with it.

@aristath
Copy link
Member

aristath commented Jan 7, 2021

@aristath thanks for the follow up on this feature! It seems like the merge is only awaiting your validation for the requested changes. Once it's merged and part of an update (not sure of the flow here) I'll be able to test it and give feedback if I find some problems with it.

I was part of this feature's development so I can't (ethically) review it, I wouldn't be objective. We'll wait for someone else to properly review it when it's time 👍

@@ -72,6 +72,7 @@ function ColumnsEditContainer( {
} );
const innerBlocksProps = useInnerBlocksProps( blockProps, {
allowedBlocks: ALLOWED_BLOCKS,
allowedChildBlocks,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line can be removed, allowedChildBlocks isn't a recognized property for inner blocks:
https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/inner-blocks/README.md#props

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree that this line should be removed.

@talldan
Copy link
Contributor

talldan commented Jan 13, 2021

@JesserH From testing, I think allowedChildBlocks is misleading, because it sounds like it'd allow an implementor to define the allowed blocks once at the parent columns level. I experimented with the following template:

function myplugin_register_book_post_type() {
	$args = array(
		'public'        => true,
		'label'         => 'Books',
		'show_in_rest'  => true,
		'template'      => array(
			array(
				'core/columns',
				array(
					'allowedChildBlocks' => array( 'core/paragraph' ),
				),
				array(
					array(
						'core/column',
					),
					array(
						'core/column',
					),
				),
			),
		),
	);
	register_post_type( 'book', $args );
}
add_action( 'init', 'myplugin_register_book_post_type' );

This didn't work, I still need to specify allowedBlocks on each column. It looks like all allowedChildBlocks does is control the allowed blocks for newly added columns.

It could be renamed, but I think this needs some more thought. I'd be inclined to strip this PR back to just include the changes to the column block allowedBlock attribute, and explore the columns block changes in a separate PR.

Also cc'ing @jorgefilipecosta and @mcsf for a review, as it looks like Jorge worked on the template lock for column and Miguel code reviewed.

Copy link
Member

@jorgefilipecosta jorgefilipecosta 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 the allowedBlocks attribute in columns is good addition, it follows the same approach as in #26128 👍

Regarding the mechanism to control allowed blocks for newly created columns I agree with @talldan, it seems safer to not include it for now.
I guess if we needed it after we can create something with specific names saying that is for new columns that covers, allowedBlocks and templateLock.

@@ -72,6 +72,7 @@ function ColumnsEditContainer( {
} );
const innerBlocksProps = useInnerBlocksProps( blockProps, {
allowedBlocks: ALLOWED_BLOCKS,
allowedChildBlocks,
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree that this line should be removed.

@JesserH
Copy link
Contributor Author

JesserH commented Jan 18, 2021

@jorgefilipecosta I updated the PR accordingly 😄

@talldan
Copy link
Contributor

talldan commented Jan 19, 2021

@JesserH The changes seem the opposite to what was recommended, I think it'd be best to keep only the changes on the column block.

@JesserH
Copy link
Contributor Author

JesserH commented Jan 22, 2021

@talldan Sorry, that's my bad 😅 . I have updated the PR 😄

@gziolo gziolo requested a review from a team January 26, 2021 07:17
@gziolo gziolo added the [Block] Columns Affects the Columns Block label Jan 26, 2021
@gziolo gziolo requested a review from mtias January 26, 2021 07:18
Base automatically changed from master to trunk March 1, 2021 15:44
@LucaPipolo
Copy link

Any update to this? Would be really great to have this feature! 🙏🏼 😬

Copy link
Contributor

@draganescu draganescu 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 this is updated according to the requested changes now, right @talldan?

@@ -67,6 +72,7 @@ function ColumnEdit( {
} );
const innerBlocksProps = useInnerBlocksProps( blockProps, {
templateLock,
allowedBlocks,
Copy link
Member

Choose a reason for hiding this comment

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

What happens when allowedBlocks is undefined?

@aristath
Copy link
Member

aristath commented Oct 5, 2021

Unfortunately we can't push any fixes or update the PR since it's on a fork, and @JesserH has since moved on to other projects.
In order to move this forward, I'm closing this PR and re-submitting it in #35342
The new PR uses this branch as a starting point, and I simply rebased it, resolved conflicts etc. I'll be addressing the feedback there. Feel free to comment on the new PR, review and push additional changes as needed. The new PR is from the main repo and not a fork so it will be easier to move forward. 👍

@aristath aristath closed this Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Columns Affects the Columns Block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Individual Column(s) should be able to define allowed blocks
9 participants