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.

Sign up for GitHub

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

Gutenberg v5.9.x and Contact Form Block Support #12673

Merged
merged 2 commits into from
Jun 14, 2019
Merged

Gutenberg v5.9.x and Contact Form Block Support #12673

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
51469f9
Test if this fixes api save validation
gwwar Jun 12, 2019
a475ada
Wrap InnerBlocks.Content in a function
gwwar Jun 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions extensions/blocks/contact-form/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export const settings = {
},

edit: JetpackContactForm,
save: InnerBlocks.Content,
Copy link

Choose a reason for hiding this comment

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

It does seem to me that this is a valid usage as we always considered save as a "component". Should we update the check function @gziolo?

Copy link
Member

Choose a reason for hiding this comment

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

In getSaveElement we have https://github.com/WordPress/gutenberg/blob/master/packages/blocks/src/api/serializer.js#L68-L70:

	// Component classes are unsupported for save since serialization must
	// occur synchronously. For improved interoperability with higher-order
	// components which often return component class, emulate basic support.

We provide a workaround, so we should relax this check. I will open PR soon.

Copy link
Member

@gziolo gziolo Jun 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It seems like this can't be the issue as we have the same check for save method almost forever:
https://github.com/WordPress/gutenberg/blob/da8184a055d8b2d40ba0306faf7103c0a961726a/blocks/api/registration.js#L96-L101

	if ( ! settings || ! isFunction( settings.save ) ) {
		console.error(
			'The "save" property must be specified and must be a valid function.'
		);
		return;
	}

My bet that is that the way deprecation works for InnerBlocks in @wordpress/editor, it doesn't propagate Content further, we need to fix it. It's clearly undefined when loaded in Jetpack. I'm sure that it will break other plugins as well.

Copy link

@youknowriad youknowriad Jun 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@gziolo Oh, that's a bigger issue that we may even consider for 5.2.x.

Update: I guess the deprecation were only introduced in 5.9 which means we're good sorry for the alarm :)

Copy link
Member

Choose a reason for hiding this comment

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

Opened WordPress/gutenberg#16152 to address the issue where statics were not hoisted for deprecated components. This should fix the issue which happens here.

Copy link
Member

Choose a reason for hiding this comment

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

In my testing, you will have to use function expression for this block for safety. It's fragile to assume that InnerBlocks.Content will always be a function. The same applies to all save implementations. Even if we allow objects, it still might work differently than intended.

Related Slack discussion on WordPress.org: https://wordpress.slack.com/archives/C02QB2JS7/p1560500199068200

Copy link

@youknowriad youknowriad Jun 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@gwwar I think we might still need this change in Jetpack because the component is not a function anymore.

save: () => <InnerBlocks.Content />,
deprecated: [
{
attributes: {
Expand Down Expand Up @@ -107,7 +107,7 @@ export const settings = {
return true;
},

save: InnerBlocks.Content,
save: () => <InnerBlocks.Content />,
},
],
};
Expand Down