-
Notifications
You must be signed in to change notification settings - Fork 842
Forms: Fix dark theme outline form #45984
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?
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 1 file.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
edanzer
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.
What we're trying to do here makes sense for outline forms. And it tests well. I installed a range of dark themes to test (see video below), and tested against white themes.
Having struggled with this kind of theme CSS consistency, I'd wouldn't be surprised if we eventually find some other use cases where the new rules break down. But that's just part of development in the WordPress space.
What I'm seeing on a range of dark forms with different form colors/settings:
https://github.com/user-attachments/assets/14ef2b52-2d3d-427b-bf47-c01366c01f4d
| '--jetpack--contact-form--border': border, | ||
| '--jetpack--contact-form--border-color': borderColor, | ||
| '--jetpack--contact-form--border-size': borderWidth, | ||
| '--jetpack--contact-form--border-size-outline': borderWidthOutline, |
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 this is meant to be a fallback for Outlined style I'd say let's use the same wording "outlined" (specially since outline is a different CSS property). Easy fix though.
| backdropFilter: inputBackdropFilter, | ||
| } = window.getComputedStyle( inputNode ); | ||
|
|
||
| const borderWidthOutline = |
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.
Do we actually need this var? By its definition and usage (if I'm not mistaken) we could use border-width: max( var( --jetpack--contact-form--border-size, 0 ), '1px' ); instead.
Would be good to test this with strange values though.
|
Wondering now if we should be trying to control input background color so hard. Editor seems to be doing fine with the default (field), and we've been trying so hard to probe the "right" color when, maybe, we don't need to set anything unless it's been set from the editor. Maybe it would all be better if we let it be whatever the background color is and detect it only for blending purposes, not to forcibly set the input background per se. I have to test that theory though and that's gonna take me on a long road of multiplicity of themes, field types and form styles :D |
|
Some themes to keep testing that have been problematic:
|
Related to FORMS-299
This PR addresses some of the default behaviour of outline forms. To make them work more like expected.
Proposed changes:
Other information:
Jetpack product discussion
This PR is related to #45866
When I was working on #45866 I noticed that the outline form doesn't quite work as expected.
The solution was getting to complex so I decided to create a new PR for the outline form to make it work more like it should work.
Does this pull request change what data or activity we track or use?
No
Testing instructions:
You will need a few different variations of the outline form.
All on the same page.
Create a default one.
Create one with plate colors that are coming from the form.
Create one with custom colors that are coming from the form.
Create one with palate colors coming from the input fields.
Create one with custom colors coming from the input fields.
Add the following inputs.
Default inputs. ( text, email, name, textare )
Phone field with and without international fields.
Image selection.
Single Checkboxes. Consent and regular checkboxs.
Radio buttons.
file upload
switch between different themes. ( not that the palate color might produce unreadable results) go to the editor and make sure that in the editor you see the same "unreadable error".