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

Contact Form: Add Gutenberg based form builder blocks #9452

Closed
wants to merge 2 commits into from

Conversation

lsl
Copy link
Contributor

@lsl lsl commented May 1, 2018

Status: In Progress

Adding a Gutenberg form builder to Jetpack for use in the contact form module.

There are a couple of issues blocking any kind of usable solution being completed, outlined further below.

Changes proposed in this Pull Request:

  • Add form block based on the InnerBlock component
  • Add text, textarea, and button form builder element blocks.

Testing instructions:

  • Add a Form block to a Gutenberg page or post.
  • Default Form should contain
    • Name, Email textboxes
    • Message textarea,
    • Submit Button
  • Ability to add form elements should only be enabled against the form block (fix for this is blocked)
  • Text elements should have a default label, no placeholder, and be required.
  • Buttons should have a default label

Proposed changelog entry for your changes:

  • Added Gutenberg form builder blocks for the contact form module.

Further info:

Blocks:

  • Form - The main block, consists of an InnerBlock for the <form>
    element and a set template consisting of a default set of inner blocks
    preconfigured as a contact form
  • Text - General <input type=text>/<textarea> field with configurable
    label, rows
  • Button - A <button type=submit> with configurable button text

Blocking Release:

Todo - Integration with grunion-contact-form:

  • Captcha support should be relatively easy to achieve with a serverside
    rendered block
  • Serverside controlled form action/method's - probably best implemented withAPIData

Todo - Handle duplicate field names gracefully:

  • Currently uses a variation on the label / field name to set
    id / for settings on label / inputs.
  • Currently uses same variation for <input name=""> attributes

How it looks:

George Stephanis and others added 2 commits May 1, 2018 21:49
At this point it just represents the parent form -- it doesn't handle any fields yet.

That will largely depend on how WordPress/gutenberg#3745 progresses.
Adds a set of Gutenberg "Form" blocks for use with the contact form
module.

Blocks:

* Form - The main block, consists of an InnerBlock for the <form>
element and a set template consisting of a default set of inner blocks
preconfigured as a contact form

* Text - General <input type=text>/<textarea> field with configurable
  label, rows

* Button - A <button type=submit> with configurable button text

Blocking Release:

* InnerBlock templating requires WordPress/gutenberg#5452.

* WordPress/gutenberg#5452 needs to override isPrivate settings to
  prevent form blocks being seen in the inserter when out of form
  context.

* WordPress/gutenberg#5452 is whitelist only meaning we cant add
  extra blocks in between form elements - not really a huge issue
  for this but likely an issue for other use cases.

Todo - Integration with grunion-contact-form:

* Captcha support should be relatively easy to achieve with a serverside
  rendered block

* A server side filter / hook could replace the form action

Todo - Handle duplicate field names gracefully

* Currently uses a variation on the label / field name to set
  id / for settings on label / inputs.

* Currently uses same variation for <input name=""> attributes
@lsl lsl requested review from georgestephanis and a team as code owners May 1, 2018 12:58
Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

Overall, I really like the approach you're taking here by using InnerBlocks. Nice work.

I left a few mostly minor comments. Note that many of them (e.g. missing key props, missing localisation) apply to all four JSX files added in this PR.

I'd like to see the contact form functionality itself implemented, e.g. one should be able to send an email by submitting the form on the frontend. Once that works we'll be in a good position to refine this PR into something shippable.

const inputName = (str) => str.toLowerCase().replace(/[^a-z0-9]/, '');

// Define the form block
let FormButton = {
Copy link
Member

Choose a reason for hiding this comment

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

We should communicate that we don't intend for this variable's value to change by declaring it const.


save: ( { attributes, className } ) => {
return <div className={ className }>
<Button isPrimary={ true } type={ "submit" }>
Copy link
Member

Choose a reason for hiding this comment

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

}

return [
<TextControl
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to specify a key prop on the <TextControl> since it is contained within a list.

Better yet, we could just omit the list altogether:

return (
	<TextControl
		...
	/>
);

},
edit: ( { attributes, setAttributes, className, isSelected } ) => {
if ( ! isSelected ) {
return <div className={ className }>
Copy link
Member

Choose a reason for hiding this comment

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

JSX is easier to read, I think, when opening and closing tags align. We can do this by wrapping multiline JSX with parentheses.

if ( ! isSelected ) {
	return (
		<div className={ className }>
			...
		</div>
	);
}

@@ -0,0 +1 @@
.wp-block-jetpack-form-button {}
Copy link
Member

Choose a reason for hiding this comment

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

Are these files necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, development artifacts to remove.

/>,

// Display on Focus
!! isSelected &&
Copy link
Member

Choose a reason for hiding this comment

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

We can remove this check. WordPress/gutenberg#5029 made <InspectorControls> check for isSelected automatically.

// The id needs to be unique for for="" but also needs to be saved for block validation
const id = 'jetpack-form-text-input-' + inputName( attributes.label );

return <BaseControl label={ attributes.label } id={ id }>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think using <BaseControl> in save() is a good idea. If, down the track, we modify the markup within BaseControl, Gutenberg will be unable to parse existing jetpack/form-text blocks.

I recommend instead that we manually render the necessary <label> markup here.


edit: ( { attributes, setAttributes, className, isSelected } ) => {
if ( ! isSelected ) {
return FormText.save( { attributes: attributes, className: className } )
Copy link
Member

Choose a reason for hiding this comment

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

Clever! I think it would be clearer, though, if we split this out into its own component that both save() and edit() use.

function FormTextInput( { attributes, className } ) {
	return ...;
}

const FormText = {
	save( { attributes, className } ) {
		return <FormTextInput attributes={ attributes } className={ className } />;
	},

	edit( { attributes, setAttributes, className, isSelected } ) {
		if ( ! isSelected ) {
			return <FormTextInput attributes={ attributes } className={ className } />;
		}
	},
	
	...
};

}

return [
<TextControl
Copy link
Member

Choose a reason for hiding this comment

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

This is missing a key prop. Alternatively, we could use Fragment (exposed as wp.element.Fragment) instead of a list.


if ( "form" == $name ) {
$namespace = "jetpack/form";
}
Copy link
Member

Choose a reason for hiding this comment

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

There's some mixed indentation going on here.

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Contact Form [Status] In Progress [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels May 2, 2018
@MichaelArestad
Copy link
Contributor

I think this needs a rebase before I can test it. There was a bug introduced to Jetpack that bricks the admin screen for me.

@MichaelArestad
Copy link
Contributor

I'm having some issues testing this branch. I've got the branch checked out. I have the required Gutenberg branch checked out. I confirmed on the server, but I'm getting this message:

image

JETPACK__VERSION
);

register_block_type( $type, array(
Copy link
Contributor

Choose a reason for hiding this comment

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

$type is undefined here.

@oskosk
Copy link
Contributor

oskosk commented May 11, 2018

To check this PR (which comes from a fork of the Jetpack repo) when working on a local clone of the Automattic/Jetpack repo:

git fetch origin pull/9452/head:pr/9452
git checkout pr/9452
yarn build

Make sure Gutenberg is installed and Jetpack is connected.

@oskosk oskosk added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Jul 11, 2018
@oskosk oskosk added this to the 6.4 milestone Jul 11, 2018
@oskosk
Copy link
Contributor

oskosk commented Jul 11, 2018

@lsl could you create a version of this branch but on this same repo instead of being a branch on a fork ?

lsl added a commit that referenced this pull request Jul 12, 2018
Context: #9452

@oskosk Requested this unfinished branch to be pushed up to
Automattic/jetpack for use in the Jetpck Beta Tester Plugin.

--

Adds a set of Gutenberg "Form" blocks for use with the contact form
module.

Blocks:

* Form - The main block, consists of an InnerBlock for the <form>
element and a set template consisting of a default set of inner blocks
preconfigured as a contact form

* Text - General <input type=text>/<textarea> field with configurable
  label, rows

* Button - A <button type=submit> with configurable button text

Blocking Release:

* Fix issues identified in original PR #9452

* InnerBlock templating requires WordPress/gutenberg#5452.

* WordPress/gutenberg#5452 needs to override isPrivate settings to
  prevent form blocks being seen in the inserter when out of form
  context.

* WordPress/gutenberg#5452 is whitelist only meaning we cant add
  extra blocks in between form elements - not really a huge issue
  for this but likely an issue for other use cases.

Todo - Integration with grunion-contact-form:

* Captcha support should be relatively easy to achieve with a serverside
  rendered block

* A server side filter / hook could replace the form action

Todo - Handle duplicate field names gracefully

* Currently uses a variation on the label / field name to set
  id / for settings on label / inputs.

* Currently uses same variation for <input name=""> attributes
@lsl
Copy link
Contributor Author

lsl commented Jul 12, 2018

@lsl lsl closed this Jul 12, 2018
@oskosk
Copy link
Contributor

oskosk commented Jul 12, 2018

Thank you @lsl !

@kraftbj kraftbj removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Contact Form [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants