Form: Fix form block alignment by removing contact form wrapping <div> element
#41274
Conversation
talldan
added
[Type] Bug
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
bot
added
[Block] Contact Form
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
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:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
github-actions
bot
added
the
[Status] Needs Author Reply
| 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 ] | ||
| ); |
There was a problem hiding this comment.
withInstanceId, withSelect and withDispatch moved to hooks.
| const wrapperRef = useRef(); | ||
| const innerRef = useRef(); |
There was a problem hiding this comment.
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.
|
|
||
| const renderSubmissionSettings = () => { |
There was a problem hiding this comment.
This was only used in one place, so I moved it inline.
| ); | ||
| }; | ||
|
|
||
| const renderVariationPicker = () => { |
There was a problem hiding this comment.
Again, this was only used in one place, so it's moved inline.
There was a problem hiding this comment.
I think it would be even better to make this a separate component too. Half of the calls to store are for the picker.
| </div> | ||
| ); | ||
| } else { | ||
| const style = window.jetpackForms.generateStyleVariables( innerRef.current ); |
There was a problem hiding this comment.
This is the withStyleVariables HOC moved in line. It's nice that it can be moved into this branch of the if statement.
| defaultVariation: getDefaultBlockVariation && getDefaultBlockVariation( props.name, 'block' ), | ||
| variations: getBlockVariations && getBlockVariations( props.name, 'block' ), | ||
| return ( | ||
| <ThemeProvider targetDom={ wrapperRef.current }> |
There was a problem hiding this comment.
This is withThemeProvider moved inline.
| <div className={ formClassnames } style={ style } ref={ innerRef }> | ||
| <InnerBlocks |
There was a problem hiding this comment.
Can probably also reduce these two <div> elements down to one with useInnerBlocksProps.
There was a problem hiding this comment.
That would be good to do. There are just two css rules to change too with block-editor-inner-blocks.
talldan
assigned tellthemachines, ntsekouras and aaronrobertshaw and unassigned talldan and tellthemachines
talldan
requested review from
tellthemachines,
ntsekouras and
aaronrobertshaw
talldan
changed the title
<div> element<div> element
| 'contact-form/salesforce-lead-form' | ||
| ]; | ||
| return { | ||
| blockType: getBlockType && getBlockType( name ), |
There was a problem hiding this comment.
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.
| } = useSelect( | ||
| select => { | ||
| const { getBlockType, getBlockVariations, getDefaultBlockVariation } = | ||
| select( 'core/blocks' ); |
There was a problem hiding this comment.
Nit: we can start using store as blockEditorStore, etc..
| <Button variant="secondary" onClick={ () => setIsPatternsModalOpen( true ) }> | ||
| { __( 'Explore Form Patterns', 'jetpack-forms' ) } | ||
| } | ||
| } else if ( ! hasInnerBlocks && registerBlockVariation ) { |
There was a problem hiding this comment.
Let's remove registerBlockVariation. Related comment
There was a problem hiding this comment.
@ntsekouras Thanks so much for the comments. I can work on another follow up PR to address these.
|
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. |
There was a problem hiding this comment.
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!
github-actions
bot
removed
[Status] Needs Author Reply
Fixes #41202
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
withThemeProviderHOC. That HOC sets lots of CSS vars on this wrapper div. Removing this HOC and usingThemeProviderinline makes it easier to pass atargetDomprop 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:
Does this pull request change what data or activity we track or use?
No
Testing instructions: