-
Notifications
You must be signed in to change notification settings - Fork 818
Forms: options block layout tweaks #43531
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: update/migrate-form-fields-to-inner-blocks
Are you sure you want to change the base?
Forms: options block layout tweaks #43531
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 🤖 🔴 Action required: We would recommend that you add a section to the PR description to specify whether this PR includes any changes to data or privacy, like so:
🔴 Action required: Please add missing changelog entries for the following projects: Use the Jetpack CLI tool to generate changelog entries by running the following command: Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
top: calc( var(--jetpack--contact-form--border-top-size, var(--jetpack--contact-form--border-size)) * -1 ); | ||
top: calc(var(--jetpack--contact-form--border-size) * -1); | ||
transform: translateY(-50%); | ||
font-size: 0.8em; | ||
} | ||
|
||
/* Form that upgrade to use inner blocks look to the global styles border size.*/ | ||
.contact-form .is-style-outlined .wp-block-jetpack-input-wrap .notched-label:has(~ .grunion-field:focus) .notched-label__label, | ||
.contact-form .is-style-outlined .wp-block-jetpack-field-select.wp-block-jetpack-input-wrap .notched-label .notched-label__label { | ||
top: calc( var(--jetpack--contact-form--border-top-size, var(--jetpack--contact-form--border-size)) * -1 ); | ||
} |
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 to explain the rationale here... This change addresses an inconsistency.
Unmigrated forms do not display global styles, for example background colors and border styles.
This is because unmigrated trunk forms do not have inner blocks, that is, they don't include jetpack/input
or jetpack/options
.
That means unmigrated forms do not "inherit" global styles meant for inner blocks.
This outlined CSS rule controls the position of the labels when the input is focussed. In the refactored form, the input border size directly affects the label position. This includes any input border sizes set in global styles/theme.json.
Before, the unmigrated forms' use of --jetpack--contact-form--border-top-size
meant that this single global style had an effect on the form style, even when others didn't.
So if you fiddled with border size, color, width, radius, background color etc in global styles for jetpack/input
or `jetpack/options, none of these have an effect on unmigrated forms. But in outlined forms the size "bled" through to the label. Therein lies the inconsistency.
We could build in a way to backfill legacy input styles with global styles, but that doesn't exist, hence the reason behind this change.
$legacy_border_size = ! empty( $border_width_attribute ) || $border_width_attribute === '0' ? $border_width_attribute . 'px' : null; | ||
|
||
$border_radius_attribute = $this->get_attribute( 'borderradius' ); | ||
$legacy_border_radius = ! empty( $border_radius_attribute ) || $border_radius_attribute === '0' ? $border_radius_attribute . 'px' : $outline_styles['border']['radius'] ?? null; |
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.
We were assigning var + px
twice.
* a limit of `100px` to the radius on the x axis. This achieves the same look and feel as other fields | ||
* that use the notch html (`notched-label__leading` has a max-width of `100px` to prevent it from getting too wide). | ||
* It prevents large border radius values from disrupting the look and feel of the fields. | ||
*/ |
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 comment formatting
* checkbox is checked. Unlike radio buttons, for which the required attribute is satisfied if | ||
* any of the radio buttons in the group is selected, adding a required attribute directly to | ||
* a checkbox means that this specific checkbox must be checked. | ||
*/ |
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.
Comment formatting only
🤔 I'm personally not sure about this as the expected behavior. It means that if I have two forms in separate places on a site, one before the inner blocks work and one added after, they might end up looking very different. It'd be very difficult for a user to figure out why that's the case and fix the issue. What are the repercussions if we do the opposite and allow global styles to apply to old forms? Is it technically feasible? |
Yeah, I agree that it could cause confusion. My only goal in this PR was to make things consistent with the base inner blocks refactor branch , which applies block supports styles/correct block classes only if the inner blocks are found. Based on the base feature branch, I took the overarching goal to always have been to "not break users' existing forms".
I don't know how technically feasible it'd be. I mean to say, it'd be possible, but the effort involved could be medium or it could be large. Especially for the outlined style, which needs to know about borders and border radii. Here, I believe, because these forms don't know about inner blocks, we'd have to map existing fields to their inner block counterparts. So, if we come up against a Apart from that, off the top of my head:
If you feel strongly about it, it might be worth a spike. |
Yeah, ok, lets make it a separate exploration. I've made an issue (FORMS-88) to discuss this. |
@@ -418,7 +418,7 @@ | |||
|
|||
&[type="radio"]::before { | |||
border-radius: 50%; | |||
transform: translateY(15%); | |||
transform: translateY(0.15em); |
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.
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.
I think the actual input element is supposed to be hidden by some css (appearance: none
) in favour of a pseudoelement. Maybe there's something overriding that in some classic themes.
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 was actually due to incoming styles from the classic themes. The jetpack form sets the width/height to be 1em
, and adds borders and things.
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.
I think I found out why: https://github.com/Automattic/jetpack/pull/43531/files#r2101527871
I compared with trunk and it fixes the 3d effect. I believe it has to do with the change to 1em
sizing for the input field, also we removed the border: none
override.
🤞🏻
… and maintainability. Update CSS for radio and checkbox options to use consistent alignment, and streamline fieldset and div class attributes by removing unnecessary prefixes. Improve handling of border styles and attributes for outlined forms, ensuring compatibility with global styles.
c0019a5
to
20eead8
Compare
min-width: auto; | ||
width: 1em; | ||
height: 1em; | ||
box-shadow: none; |
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 is to avoid some really weird 3d effects on classic themes, but testing in trunk they're not there.
I'll look into a better fix.
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.
Uh, here's the main feature branch so I think it's something we do need to address:

I'll leave these styles then here.
What's happening is that we added width/height of 1em
so the inputs scale with the font size.
We also removed the border and height overrides: https://github.com/Automattic/jetpack/pull/42890/files#diff-b032342e36da30ca1c7a18a770aac75a9d52bdec892fe09e3e047c3a8ca73b69L368
So I think we need to keep these new declarations here to compensate.
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.
Not removed, it was moved to here to lower the specificity:
border: none; |
That looks like a mistake. Maybe these rules should all have !important
if the border/outline is never to be shown. 🤔
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.
Probably the same with appearance: none
as well, if that's overridden by a theme's css then border/outline being set to none
won't have any effect.
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.
Maybe these rules should all have !important if the border/outline is never to be shown.
That the border/outline is never to be shown was my assumption after testing in trunk. At least the contact form CSS is opinionated on the matter. 😄
What's your recommendation here? Should I move the following from :where(.jetpack-option__type) {
to .jetpack-option__type.jetpack-option__type
:
height: 1em;
width: 1em;
border: none;
And give them an !important
?
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.
Yep, I think !important
might be the best choice for those, and also the outline
and appearance
. Just need to re-test to make sure it doesn't affect the application of global styles and block styles in any way.
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 as long as the font-size
rule is still at 0-1-0 specificity it should be ok.
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.
Done in a5a6b2b
Looks pretty consistent in my testing.
The only thing I noticed was the mismatch in label/input colors on the option block, but I think you're looking at that.
@@ -569,10 +567,6 @@ | |||
margin: 0; | |||
} | |||
|
|||
.jetpack-option__type.jetpack-option__type { |
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 selector was duplicated above.
Even though the above selector is nested in .jetpack-field
, I don't see any control where .jetpack-option__type
is outside a field.
Proposed changes:
Important
This PR's base is #42890
Fixes: https://github.com/Automattic/jetpack-roadmap/issues/2501
This PR:
--jetpack--contact-form--border-size
now that we have--jetpack--contact-form--border-*-size
to handle global styles. This ensures legacy forms use their original values.wp-block-jetpack-options
where the inner block exists. This makes the implementation consistent withwp-block-jetpack-input
TODO
Testing instructions:
For the checkbox layout
Create a default form with some multi fields (checkbox and radio). Play around with the option text length and font size.
Ensure that the option input aligns with the text at the top of the parent container. There will be a pixel off here and there depending on the font size, but it should be an improvement on trunk.
For the CSS var
Create a few forms in trunk, all with options blocks (checkbox and radio fields). No need to style them.
Now switch to this branch.
Add some border color, width and radius styles to the
jetpack/options
block in global styles.Check that your forms built in trunk are unaffected on the frontend
For the default padding
Create a default form with some multi fields (checkbox and radio). With your global styles for the
jetpack/options
block still active, visit the frontend. The multi fields should have left padding via thejetpack-field-multiple__list--has-border
class.