-
Notifications
You must be signed in to change notification settings - Fork 817
Forms: fix inconsistent weights in outline style and terms labels #43545
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: fix inconsistent weights in outline style and terms labels #43545
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! |
507e24c
to
d364658
Compare
d364658
to
742cd8c
Compare
} | ||
|
||
|
||
:where(.contact-form label.consent) { |
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'm aware you moved the specificity back in 0791de8, though — these specificity changes sure are tricky to change! 😓
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.
Oh dang. I'm not sure what the right thing to do is here.
How much should we pay attention to the styles of every theme? I'm not sure it's a winning game.
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 for the consent field it's okay to raise specificity. I think the only major result is that global styles changes to label typography will be overwritten. 🤔
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.
In this case, it's the grunion
CSS that appears to be winning out and setting font-weight: 700
at least?
How much should we pay attention to the styles of every theme? I'm not sure it's a winning game.
Me either. In contrast to what we might do in core, I think Jetpack being opinionated is fine, and if themes set general rules for how label
s should appear, I think that's probably okay, too.
From testing locally, I think the consent specificity can be upped back to .contact-form :where(label.consent)
as setting the font-weight in global styles winds up being output as :root :where(.wp-block-jetpack-option) {
which seems to win out?

So in this case, I think global styles rules still work okay with slightly upped specificity, but I very well might be missing something in my testing!
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.
Thank you for testing that!
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.
Done in c875ed7
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.
All testing well for me, now! Might be worth double checking that this doesn't conflict with @talldan's specificity work, but otherwise looks good to merge in to me 👍
Yep, I'm reviewing now if you can hold off merging. At a first glance a lot of these selectors seem too high/low specificity. 0-1-0 should be the rule of thumb. |
font-weight: 400; | ||
} | ||
.jetpack-field-consent, | ||
.jetpack-field-checkbox .jetpack-field-label .jetpack-field-label__input { |
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.
From what I can tell this selector isn't doing anything. I guess it's the same selector that was used in trunk, but what I'm not sure about is if it's working in trunk due to the different markup.
It's very high specificity, so if it did work it'd probably break global styles. 😅
I think we can either remove it, or fix it and change it to 0-1-0 specificity
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 for clarification, it's the .jetpack-field-checkbox .jetpack-field-label .jetpack-field-label__input
one that I'm referring to.
@@ -1065,6 +1066,10 @@ | |||
font-weight: 700; | |||
} | |||
|
|||
:where(.is-style-outlined .wp-block-jetpack-label) { |
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 making the specificity 0-0-0. Elsewhere we're using 0-1-0 specificity for these kind of styles, the rationale being that we want to override css resets which are often 0-0-1, but still stay low enough for global styles and block styles so that they can override.
Hard to say if a theme would change the label font size, but I think a small bump in specificity is safest.
:where( .grunion-checkbox-multiple-options legend ), | ||
:where( .grunion-radio-options legend ) { |
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 rationale for reducing to zero? In general I think it should also be aiming for 0-1-0 for consistency, otherwise themes have a hard time working around 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.
No rationale. I'm playing Whac-A-Mole. 😄
0-1-0 for consistency
I'll fix this, thanks for confirming!
Fixes FORMS-41
Proposed changes:
jetpack/option
)Testing instructions:
Create an outlined form block with:
In global styles, update the typography appearance for the
jetpack/label
block to bold and italic or some other conspicuous combo.Save that. Check that the frontend and editor are consistent.
Before (editor wasn't displaying the global font weight)
After (it now does)
Now reset your global styles for the label block.
Head to the post editor and apply a typography appearance to the label.
Check that the frontend and editor are consistent.
Before
After
Also check that the terms and conditions label has a weight of
400
in both the frontend and editor. The frontend has always been 400.Before
After