Form: Fix form block alignment by removing contact form wrapping `<div>` element by talldan · Pull Request #41274 · Automattic/jetpack

talldan · 2025-01-23T07:59:56Z

Proposed changes:

Removes the wrapping <div> element from the form block, and tidies up the component, removing all use of Higher Order Components.

Doing this should fix wide/full alignments for the form block.

The wrapping div was being inserted by the withThemeProvider HOC. That HOC sets lots of CSS vars on this wrapper div. Removing this HOC and using ThemeProvider inline makes it easier to pass a targetDom prop with a reference to the block wrapper element, and it results in the CSS vars being added to the block wrapper instead.

After that I moved all the other HOCs inline, as they add quite a lot of unnecessary complexity.

This is best reviewed with white space hidden, as there's a big indentation change, it makes the diff a lot less intimidating!

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Does this pull request change what data or activity we track or use?

No

Testing instructions:

Add the contact form block
Select a variation
Try changing the form to wide/full alignments and see that it now works
Try changing a range of styles on the form and on the inner elements
Switch between trunk and this branch and compare whether it works the same
Let me know if you see any bugs!

…line

github-actions · 2025-01-23T08:00:54Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the remove/contact-form-hocs branch.

To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack remove/contact-form-hocs

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

github-actions · 2025-01-23T08:00:58Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add a "[Type]" label (Bug, Enhancement, Janitorial, Task).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Choose a review path based on your changes:
- A. Team Review: add the "[Status] Needs Team Review" label
  - For most changes, including minor cross-team impacts.
  - Example: Updating a team-specific component or a small change to a shared library.
- B. Crew Review: add the "[Status] Needs Review" label
  - For significant changes to core functionality.
  - Example: Major updates to a shared library or complex features.
- C. Both: Start with Team, then request Crew
  - For complex changes or when you need extra confidence.
  - Example: Refactor affecting multiple systems.
Get at least one approval before merging.

Still unsure? Reach out in #jetpack-developers for guidance!

Jetpack plugin:

The Jetpack plugin has different release cadences depending on the platform:

WordPress.com Simple releases happen semi-continuously (PCYsg-Jjm-p2).
WoA releases happen weekly.
Releases to self-hosted sites happen monthly. The next release is scheduled for February 4, 2025 (scheduled code freeze on February 4, 2025).

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

talldan · 2025-01-23T08:03:42Z

+	const instanceId = useInstanceId( JetpackContactFormEdit );
+	const { replaceInnerBlocks, selectBlock } = useDispatch( 'core/block-editor' );
+	const {
+		blockType,
+		canUserInstallPlugins,
+		defaultVariation,
+		variations,
+		hasInnerBlocks,
+		postAuthorEmail,
+	} = useSelect(
+		select => {
+			const { getBlockType, getBlockVariations, getDefaultBlockVariation } =
+				select( 'core/blocks' );
+			const { getBlocks } = select( 'core/block-editor' );
+			const { getEditedPostAttribute } = select( 'core/editor' );
+			const { getUser, canUser } = select( 'core' );
+			const innerBlocks = getBlocks( clientId );

-		const formClassnames = clsx( className, 'jetpack-contact-form', {
-			'is-placeholder': ! hasInnerBlocks && registerBlockVariation,
-		} );
+			const authorId = getEditedPostAttribute( 'author' );
+			const authorEmail = authorId && getUser( authorId ) && getUser( authorId ).email;
+			const submitButton = innerBlocks.find( block => block.name === 'jetpack/button' );
+			if ( submitButton && ! submitButton.attributes.lock ) {
+				const lock = { move: false, remove: true };
+				submitButton.attributes.lock = lock;
+			}

-		const isSalesForceExtensionEnabled =
-			!! window?.Jetpack_Editor_Initial_State?.available_blocks[
-				'contact-form/salesforce-lead-form'
-			];
+			return {
+				blockType: getBlockType && getBlockType( name ),
+				canUserInstallPlugins: canUser( 'create', 'plugins' ),
+				defaultVariation: getDefaultBlockVariation && getDefaultBlockVariation( name, 'block' ),
+				variations: getBlockVariations && getBlockVariations( name, 'block' ),
+				hasInnerBlocks: innerBlocks.length > 0,
+				postAuthorEmail: authorEmail,
+			};
+		},
+		[ clientId, name ]
+	);


withInstanceId, withSelect and withDispatch moved to hooks.

talldan · 2025-01-23T08:07:16Z

+	const wrapperRef = useRef();
+	const innerRef = useRef();


The wrapperRef is for ThemeProvider and its targetDom prop.

The innerRef is replacing the fowardRef that wrapped the component and was used for withStyleVariables. Now that the logic from that HOC is moved into the component, forwardRef isn't needed and an internal ref can be used instead.

talldan · 2025-01-23T08:07:33Z

+		selectBlock( clientId );
+	};

-		const renderSubmissionSettings = () => {


This was only used in one place, so I moved it inline.

talldan · 2025-01-23T08:08:03Z

 			);
-		};
-
-		const renderVariationPicker = () => {


Again, this was only used in one place, so it's moved inline.

I think it would be even better to make this a separate component too. Half of the calls to store are for the picker.

talldan · 2025-01-23T08:10:34Z

+			</div>
+		);
+	} else {
+		const style = window.jetpackForms.generateStyleVariables( innerRef.current );


This is the withStyleVariables HOC moved in line. It's nice that it can be moved into this branch of the if statement.

talldan · 2025-01-23T08:11:16Z

-			defaultVariation: getDefaultBlockVariation && getDefaultBlockVariation( props.name, 'block' ),
-			variations: getBlockVariations && getBlockVariations( props.name, 'block' ),
+	return (
+		<ThemeProvider targetDom={ wrapperRef.current }>


This is withThemeProvider moved inline.

talldan · 2025-01-23T08:12:36Z

+				<div className={ formClassnames } style={ style } ref={ innerRef }>
+					<InnerBlocks


Can probably also reduce these two <div> elements down to one with useInnerBlocksProps.

That would be good to do. There are just two css rules to change too with block-editor-inner-blocks.

ntsekouras · 2025-01-23T09:10:21Z

-				'contact-form/salesforce-lead-form'
-			];
+			return {
+				blockType: getBlockType && getBlockType( name ),


It seems there are some weird checks like this ( if selector is available ) and I believe it's just legacy code. The APIs checked are for years in WP and I believe it's safe to remove and simplify.

Another example is this effect that can be removed.

ntsekouras · 2025-01-23T09:11:27Z

+	} = useSelect(
+		select => {
+			const { getBlockType, getBlockVariations, getDefaultBlockVariation } =
+				select( 'core/blocks' );


Nit: we can start using store as blockEditorStore, etc..

ntsekouras · 2025-01-23T09:13:12Z

-						<Button variant="secondary" onClick={ () => setIsPatternsModalOpen( true ) }>
-							{ __( 'Explore Form Patterns', 'jetpack-forms' ) }
+		}
+	} else if ( ! hasInnerBlocks && registerBlockVariation ) {


Let's remove registerBlockVariation. Related comment

@ntsekouras Thanks so much for the comments. I can work on another follow up PR to address these.

simison · 2025-01-23T09:16:09Z

Let's get this in soon to avoid too much rebase hassle; some of the changes might be fine for small follow-up PRs, for example.

simison

Haven't reviewed code but did a bunch of smoke-testing, and this is working well.

Something I didn't test yet but wanted to was RTL support and if this improves things, see issue:

#40737

edanzer

Nice PR! The resulting code after these changes is so much nicer to to work with. Really nice to remove all the HOCs and modernize the code. Good tip on white space removal - was otherwise impossible to follow the code changes here.

I tested this thoroughly and was unable to 'break' form styles or cause styling issues. I'm actually a bit surprised given lots of code changes and the removal of a major parent div. But that's great.

Things I tested: inserting editing a wide range of form fields; adjusted padding, margin on the form; shifting form field into multiple columns using 50% width on the field; adjusting label and input styles; alignment settings (only the button has this I think); few different themes; RTL language in the editor (Hebrew).

There was chatter this might resolve a button alignment issue (issue, pending fix), but this PR did not have an effect on that for me.

I was also trying to think hard if and how any of these changes might cause issues with backwards compatibility for existing forms, but I can't see anything myself, particularly given the changes are confined to the editor and not the frontend for the block.

@simison - you also mentioned an issue with form styles breaking for RTL, but from what I can see that issue affects the frontend and should not be affected here? Since I was test RTL anyways, I did check frontend. Oddly I was unablee to duplicate the reported issues. Not sure if I missing something, or another PR affected that.

I would agree with @simison that it would be good to get this out quick-ish to avoid rebase issues, and maybe defer other small improvements to follow ups. Lots of pending form-related PRs.

Anyway, given everything above, looks good here!

…v>` element (#41274)

talldan added 8 commits January 23, 2025 12:43

Switch from withDispatch to useDispatch in contact form block

8ce3d46

Switch from withSelect to useSelect in contact form block

d092f8f

Switch from withInstanceId to useInstanceId and move ThemeProvider in…

1df3046

…line

Move where ThemeProvider is rendered to ensure it always wraps

79eea81

Move withStyleVariables inline and delete HOC

451243f

Move single use render functions inline, replace Fragment with <>

9379f3a

Add missing import

a07c46a

changelog

75db61a

talldan added Bug When a feature is broken and / or not performing as intended [Package] Forms labels Jan 23, 2025

talldan self-assigned this Jan 23, 2025

github-actions Bot added [Block] Form Form block (also see Form package label) [Feature] Forms [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Jan 23, 2025

github-actions Bot added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Jan 23, 2025

talldan commented Jan 23, 2025

View reviewed changes

talldan added the [Status] Needs Team Review Obsolete. Use Needs Review instead. label Jan 23, 2025

talldan assigned tellthemachines, ntsekouras and aaronrobertshaw and unassigned talldan and tellthemachines Jan 23, 2025

talldan assigned talldan and unassigned ntsekouras and aaronrobertshaw Jan 23, 2025

talldan requested review from aaronrobertshaw, ntsekouras and tellthemachines January 23, 2025 08:14

talldan changed the title ~~Remove contact form wrapping <div> element~~ Form: Fix form block alignment by removing contact form wrapping <div> element Jan 23, 2025

simison requested review from a team and monsieur-z January 23, 2025 08:56

ntsekouras reviewed Jan 23, 2025

View reviewed changes

simison reviewed Jan 23, 2025

View reviewed changes

edanzer approved these changes Jan 23, 2025

View reviewed changes

simison merged commit 7928947 into trunk Jan 24, 2025

simison deleted the remove/contact-form-hocs branch January 24, 2025 09:37

github-actions Bot removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Status] Needs Team Review Obsolete. Use Needs Review instead. labels Jan 24, 2025

github-actions Bot added this to the jetpack/14.3 milestone Jan 24, 2025

talldan mentioned this pull request Jan 28, 2025

Form: Remove old block editor back-compat code and improve code quality #41348

Merged

3 tasks

CodeyGuyDylan pushed a commit that referenced this pull request Jan 28, 2025

Form: Fix form block alignment by removing contact form wrapping `<di…

a275549

…v>` element (#41274)

This was referenced Jan 30, 2025

Forms: Fix field widths and spacing in the editor #41415

Merged

Forms: Text area, dropdown fields have incorrect styles in block style previews and when resizing the canvas #41418

Open

		<div className={ formClassnames } style={ style } ref={ innerRef }>
		<InnerBlocks

Conversation

talldan commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes:

Other information:

Does this pull request change what data or activity we track or use?

Testing instructions:

Uh oh!

github-actions Bot commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talldan Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simison commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simison left a comment

Choose a reason for hiding this comment

Uh oh!

edanzer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

talldan commented Jan 23, 2025 •

edited

Loading

github-actions Bot commented Jan 23, 2025 •

edited

Loading

github-actions Bot commented Jan 23, 2025 •

edited

Loading

talldan Jan 23, 2025 •

edited

Loading

simison commented Jan 23, 2025 •

edited

Loading

edanzer left a comment •

edited

Loading