-
Notifications
You must be signed in to change notification settings - Fork 842
Forms: add layout support #45272
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
base: trunk
Are you sure you want to change the base?
Forms: add layout support #45272
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
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 🤖 Follow this PR Review Process:
If you have questions about anything, 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. |
Code Coverage SummaryCoverage changed in 6 files. Only the first 5 are listed here.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
| $url = apply_filters( 'grunion_contact_form_form_action', $url, $GLOBALS['post'], $id, $page ); | ||
| $has_submit_button_block = str_contains( $content, 'wp-block-jetpack-button' ); | ||
| $form_classes = 'contact-form commentsblock'; | ||
| $form_classes = 'contact-form jetpack-contact-form__form'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep the "commentsblock" class? Mostly for backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can, but I haven't seen it in use anywhere, do you know where is that coming from of what is it used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is that it would be used by themes that want to style comment forms and we would apply the same styles as them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a nice guess, but it also raises the question as usual: should we rule styling over our inner blocks? And, even if we allow customization, should a "comments block" style the same as our form blocks? After all, we try hard to flow into the theme's styling. TBH I'm not sure how to outweigh a decision here. I mostly wanted to just have a class specific to our form element, so we always know which is it and can include/exclude on stylings. We can put the commentsblock class back in, but feels pretty much as a tweak or concession instead of a clear definition of what our markup should be or abide by.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I agreed with Enej, but you do have a point, and commentsblock seems very wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove it, let's remove it in a PR that's much easier to revert than this mega PR :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.
07d2a02 to
74c3d24
Compare
4802ade to
76a70cf
Compare
| const blockProps = useBlockProps( { ref: wrapperRef } ); | ||
| const blockProps = useBlockProps( { | ||
| ref: wrapperRef, | ||
| className: clsx( className, variationName === 'multistep' && 'is-multistep' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an alternative, might be a bit easier to read:
clsx(
className,
{ 'is-multistep': variationName === 'multistep' }
)|
|
||
| // Extract only valid block registration properties from block.json | ||
| // Exclude file-based properties like editorScript, style, etc. | ||
| const { editorScript, style, name: blockName, $schema, ...validBlockMetadata } = blockMetadata; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would've been a good separate PR to simplify this one. ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, but I discover the discrepancies while testing this one and creating a new PR trying to explain the reason for the change would have implied some back and forth / delays. Thought I'd just add it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Referencing a change to WIP PRs to demonstrate how it makes dev ergonomics better elsewhere is fine.
It's ok here now tho.
| index: 1, | ||
| element: ( | ||
| <> | ||
| <div key="phoneFieldControls"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it need a wrapper if children already have keys? 🤔 It's unrelated to this PR, so it would've been better in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will separate this one, it was just killing me to get an error on every reload
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // this styles are common to both editor and frontend | ||
| // Gutenberg versions will use different classname strategies | ||
| // for layout flex support, cover both possibilities here: frontend and editor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typos and not very clear comment
| // somehow we are getting the class jetpack-field__input-element on the wrong wrapper | ||
| // .jetpack-field__input-element is meant to style a div mocking an input element, not its wrapper. | ||
| $class_names = preg_replace( '/jetpack-field__input-element /', '', $class_names ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you get to the bottom of why this is happening, or it's a good follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good for follow up!
| */ | ||
| private static function render_error_wrapper() { | ||
| $html = '<div class="contact-form__error" data-wp-class--show-errors="state.showFormErrors">'; | ||
| private static function render_error_wrapper( $classes = array() ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why an array instead of a string if we only ever pass it one class in the codebase? I would simplify, and if we later need to pass array we can turn it to a string higher up before this function.
Alternatively, pass is_horizontal boolean and manage CSS class directly in the function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why an array instead of a string
Just because is more future-proof. It can easily be a string. Using a boolean would not simplify as we'd need a ternary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to single string param for extra CSS classes
| const fieldId = context.fieldId; | ||
| const field = context.fields[ fieldId ] || {}; | ||
| // when there is only one field, don't show errors until the user tries to submit the form. | ||
| // And even then, return false so we only show the general form error message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to indeed improve what we show on submit.
We should instead show the inline form message, which looks much better than the big error badge. This change would've been a good independent PR too to get it in quicker and make testing easier.
Let's also continue invalidate on-blur to save users click for on-submit, that was the whole point of that UX improvement so let's not remove it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm good catch, I need to make sure we only do this on single input, horizontal forms. Will use iApi for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simison could we do something like this for all of our forms, please? 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CGastrell prolly best we iterate on this in a separate PR to keep individual changes moving?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ilonagl, the summary error thing has some purpose on very long forms, where you don't see inline errors above the fold, but I think the UX/design can be improved a lot for sure when pressing "submit".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simison yes, agreed! We recently had a chat about with this with Enej. Do we have a Linear or Github task where I could start gathering designs and notes?
| Jetpack_Forms::plugin_url() . '../dist/contact-form/css/jetpack-contact-form.css', | ||
| array(), | ||
| \JETPACK__VERSION | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the logic with two style files? Should everything from grunion.css move here next up, are they loaded separately conditioally, or something else?
Now the two separate stylesheets don't feel like they make much sense because of how they're named.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it doesn't make "much" sense. I can easily merge this onto grunion, but that other one is so messed up it's very difficult to work with. Here we just register the style, then it's the new blocks API in charge of loading it: the frontend loads this CSS on demand (as in, not loaded unless there's a form block present, see class-contact-form-block.php:58). I thought it could become a new file to work with and, hopefully, a place to start moving our CSS while we clean up grunion. Otherwise, options are: merge onto grunion or name it specifically as part of the layout support styles.
…generic form error suffices and allows for single-field, horizontal forms (subscribe)
Co-authored-by: Mikael Korpela <[email protected]>
…t want the inlined error under the field
…d justification support
0f5e795 to
337f0a9
Compare
dhasilva
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests fine for me. Would be wise to get more reviews.


Part of FORMS-220
Proposed changes:
This PR is both a PoC to add layout support to the form block, but also intends to fix and clarify some of our under the hood CSS mess.
What it does:
.wp-block-jetpack-contact-form-container)Why:
.wp-block-jetpack-contact-form-containeras it was messing with the layout support and causing the classes to be assigned to the wrong element. So, the container class was changed and the probe selector (view.ts) to match it:.jetpack-contact-form-containerOther information:
Jetpack product discussion
p1758236622558259-slack-C052XEUUBL4
peKye1-1Lr-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Build both Jetpack and Forms:
Visit the editor and create forms with different layouts. See if they behave as expected.
Visit the frontend and see if they match the editor preview.