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 'innerBlocks' support for transforms #11979

Merged
merged 1 commit into from
Jan 29, 2019
Merged

Add 'innerBlocks' support for transforms #11979

merged 1 commit into from
Jan 29, 2019

Conversation

Luehrsen
Copy link
Contributor

@Luehrsen Luehrsen commented Nov 16, 2018

Description

This PR aims to provide support for 'innerBlocks' in transform rules. The proposed syntax is like this.

transforms: {
	from: [
		{
			type: 'block',
			blocks: ['agncy/split'],
			transform: function( attributes, innerBlocks ) {
                           return createBlock( 'wpm/split', attributes, innerBlocks );
                        },
		}
	]
},

How has this been tested?

This PR has been manually tested. E2E tests are possible and the needed plugin code exists, the test itself has not been written.

Types of changes

  • Added an attribute to the transform function to regard innerBlocks
  • Catched a possible error while rendering the block switcher where 'blocks' could be undefined due to a block not being registered

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

closes #13052

@mtias mtias added the [Feature] Block API API that allows to express the block paradigm. label Nov 16, 2018
@Luehrsen
Copy link
Contributor Author

I consider this PR more or less done from a code standpoint, as it is really minor.

However, I have not yet written (possible) e2e tests, as I have no idea how to do that. But I've updated the 'inner-blocks-templates' test plugin to add an appropriate block to test my two changes.

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.

Nice changes @Luehrsen, thank you for your contribution 👍
Things tested well and the code looks good.
Given that we already have samples created here to be used in the tests, would it be possible to add a sample to https://github.com/WordPress/gutenberg/blob/master/docs/block-api.md#transforms-optional and document this enhancement to the API?

@jorgefilipecosta jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P labels Nov 16, 2018
@Luehrsen
Copy link
Contributor Author

@jorgefilipecosta Done.

While reviewing that doc-page: Aren't the ES5 examples wrong in terms of curly-braces and 'content' in reality being 'attributes'?

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.

API-wise, this looks like a reasonable change which aligns with the general pattern of a "name, attributes, innerBlocks" triple when the name is already known.

Could you make the following changes?

  • Additional unit tests for switchToBlockType in the test/factory.js file to cover new functionality
  • A note in packages/blocks/CHANGELOG.md describing the new feature, bumping to a new minor "Unreleased" version (reference)

@Luehrsen
Copy link
Contributor Author

Luehrsen commented Jan 8, 2019

@aduth I have updated the changelog as requested, but I am a total noob in terms of TDD, so any help with the unit test would be MUCH appreciated.

packages/tests-e2e/plugins/inner-blocks-templates/index.js Outdated Show resolved Hide resolved
packages/blocks/CHANGELOG.md Outdated Show resolved Hide resolved
packages/blocks/CHANGELOG.md Outdated Show resolved Hide resolved
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 rebased and force-pushed to resolve conflicts and add missing unit tests.

Writing the test for the multi-blocks, it became apparent that it's super goofy to pass as arguments two arrays of the attributes and innerBlocks of the blocks being transformed, rather than just the array of blocks themselves. Alas, it seems what we're forced into doing to keep backward-compatibility.

@gziolo gziolo added this to the 5.0 (Gutenberg) milestone Jan 25, 2019
@youknowriad youknowriad merged commit cd0af05 into WordPress:master Jan 29, 2019
@Luehrsen Luehrsen deleted the add/innerBlocks-support-transforms branch February 20, 2019 14:04
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block transforms don't have access to inner blocks
6 participants