Forms: Add slider field #44150
Forms: Add slider field #44150
Conversation
edanzer
added
[Type] Enhancement
|
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
[Feature] Contact Form
[Plugin] Jetpack
|
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 3 files.
8 files are newly checked for coverage. Only the first 5 are listed here.
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
force-pushed
the
add/forms-slider-field
branch
3 times, most recently
from
a68cfdc to
19e5790
Compare
edanzer
force-pushed
the
add/forms-slider-field
branch
from
e4d774a to
48ae050
Compare
edanzer
force-pushed
the
add/forms-slider-field
branch
from
c397151 to
fd24f67
Compare
projects/packages/forms/src/contact-form/class-contact-form-field.php
Outdated
Show resolved
Hide resolved
projects/packages/forms/src/contact-form/class-contact-form-field.php
Outdated
Show resolved
Hide resolved
| // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped -- output is pre-escaped by method | ||
| echo wp_interactivity_data_wp_context( | ||
| array( | ||
| 'sliderValue' => $current_value, |
There was a problem hiding this comment.
Can we use the fieldValue as the slider value instead. This is already being padded into the context. In line
https://github.com/Automattic/jetpack/blob/trunk/projects/packages/forms/src/contact-form/class-contact-form-field.php#L1870
$value I think you can set the $value in two ways. One via the URL query parameter. And the second way is via the default attribute on the block. I haven't tried this yet but that worked for the ratings block. See https://github.com/Automattic/jetpack/blob/trunk/projects/packages/forms/src/blocks/field-rating/index.js#L27
There was a problem hiding this comment.
@enejb - I just pushed some change and I think I have this wired the way you're looking for now.
In the editor I no longer distinguish between the active slider value and the 'starting' value. If you set the slider at a particular number, that will also be the starting value. I'm using 'default' in the editor to do that.
On the frontend, I've updated to use fieldValue and removed some redundant interactivity directives.
Feel free to have a look and let me know if you see outstanding things need for all the basic value / default value / interactivity wiring.
|
Using https://www.smashingmagazine.com/2021/09/simplifying-form-styles-accent-color/#simple-use 
Here's another example using CSS but also note how they use mixins — we could borrow some ideas from there to simplify our scss file: https://codepen.io/thebabydino/pen/jEveqE More similar examples via https://codepen.io/collection/DgYaMj/?cursor=eyJwYWdlIjoyfQ== |
|
|
||
| .jetpack-input-range__min-label, | ||
| .jetpack-input-range__max-label { | ||
| font-size: .9em; |
There was a problem hiding this comment.
Would be good to opt-into typography settings for slider and re-use default theme font size, yet allow customization.
There was a problem hiding this comment.
There are multiple instances of text in the slider designs:
A question is if we opt the slider field into typography settings, should those settings apply to all? It probably makes sense to apply them to the numeric values and labels that appear on the left/right of the input. The small value indicator on top is trickier, because it's inversed with background and text.
| height: 2px; | ||
| background: var(--jetpack--contact-form--button-primary--background-color); | ||
| outline: none; | ||
| border-radius: 2px; |
There was a problem hiding this comment.
Could be we opt-in to support theme inherited border-radius. Not sure we should default to border radius from buttons since the elements are so different size, but that would allow more boxy looking sliders on boxy looking themes. 🤔 Anyway, we could allow customizing this in the sidebar.
There was a problem hiding this comment.
This border radius is actually for the very small line of the range input. I'm not sure default border radius for other elements makes sense there? And it seems like a very small element to allow customization?
There was a problem hiding this comment.
Gotcha!
Yeah it probably makes more sense with the badge, but more customization options are fine for a follow-up.
There was a problem hiding this comment.
And overall yes, it'll make sense to allow more boxy designs and rounded designs but we'll need to figure out how to do it:
(via)
| </InspectorControls> | ||
| <div { ...blockProps }> | ||
| <div className="jetpack-input-range-row"> | ||
| <span className="jetpack-input-range__min-label">{ min }</span> |
There was a problem hiding this comment.
Adding contenteditable="plaintext-only" feels pretty good here for allowing editing values directly in the canvas. It'll need some code to allow only digits as values and defaulting to 0 for empty content, though.
Alternatively <input type="number"> but that needs overriding all styles for the input to blend it in, but gives you up/down arrow controls for free, number keyboard on mobile, etc. nice things.
I'd probably go number input route myself.
edanzer
force-pushed
the
add/forms-slider-field
branch
from
fd24f67 to
ec5878e
Compare
edanzer
force-pushed
the
add/forms-slider-field
branch
from
be8a2d2 to
0edae74
Compare
There was a problem hiding this comment.
Looks good! And works as described.
The only case I was able to confuse the field was setting max and min settings the same value and somehow I was able to set the default to 0. Ends up in an inoperable field on the frontend, but that's up to the user to not mess up.
In any case, being beta we can follow up on any weird case we want to address
| }, | ||
| default: { | ||
| type: 'number', | ||
| default: 50, |
There was a problem hiding this comment.
Wondering it would make sense to not be opinionated about default being right in the middle, and default this also to 0?
| @@ -34,7 +34,7 @@ const JetpackField = props => { | |||
| const template = useMemo( () => { | |||
| return [ | |||
| [ 'jetpack/label', { label, required, requiredText } ], | |||
| [ 'jetpack/input', { type } ], | |||
| [ type === 'slider' ? 'jetpack/input-range' : 'jetpack/input', { type } ], | |||
There was a problem hiding this comment.
Instead of abstracting at this file, other more custom blocks abstract directly at edit.js:
https://github.com/Automattic/jetpack/blob/trunk/projects/packages/forms/src/blocks/field-file/edit.js
https://github.com/Automattic/jetpack/blob/trunk/projects/packages/forms/src/blocks/field-rating/edit.js
It results a bit more boiletplate (which we can abstract to hooks of course) but otherwise wondering if we should stick to one consistent way?
There was a problem hiding this comment.
It seems like there are a few approaches we could take.
-
Most of our fields use JetpackField, which then outputs the default Label and Input blocks. If custom logic is needed, it is handled in the Input block. For example, the Input block conditionally outputs a rich text component for dropdowns or conditionally loads min/max inspector controls for number inputs.
=> I could do this with the slider input. I could remove the code you flagged here and update Input to output slider-specific markup and inspector controls if type = slider.
=> The upside is we re-use the same components as much as possible, we tweak the Input block in ways that we're already doing, we don't have to tweak Jetpack Field, and I'd be able to drop the custom input-range component I added entirely.
=> The downside is we stretch the Input block by adding a lot of field-specific logic. -
On other side, the two you mention do not use JetpackField or the default Input block at all. They directly load their own children. This makes sense because they both load very custom inputs where using the Input block doesn't make much sense.
=> The upside is we don't don't over extend JetpackField or Input.
=> The downside is that JetpackField and Input offer a lot of built in logic for free that you need to take care to duplicate. -
I was trying a middle approach that uses JetpackField, but tweaks it to output a separate input.
=> The upside is I still get all the JetpackField logic for free, and I don't overly stretch the Input block.
=> The downside is we have field-specific logic in JetpackField now, I don't re-use the Input block, I have to create a custom input-range component, and finally this hybrid approach differs from other blocks which have generally taken option 1 or 2.
Thoughts?
There was a problem hiding this comment.
Nice summay of cons/pros! Thanks.
=> The downside is that JetpackField and Input offer a lot of built in logic for free that you need to take care to duplicate.
I'm less worried about boilerplate/duplicate logic if it results in cleaner if/else structure of code. I.e. code shouldn't feel too mangled up, and unclear when you re-use a component. What actually happens is that the component contains if/else logic at the deepest level only and does not contain an if/else fork somewhere halfway through the component.
It's hard to back out from mangled code, but easy to remove boilerplate by moving things to hooks and functions.
How would the decision look if we contained a bit more the boilerplate to avoid it? Might be helpful regardless for file uploads and ratings blocks, too.
=> The upside is I still get all the JetpackField logic for free, and I don't overly stretch the Input block.
or conditionally loads min/max inspector controls for number inputs.
Yeah, I'm not sure if this is great either code-wise and we could probably move them a bit up. I get they ended up there probably to have the settings in the number input rather than field.
| @@ -1,7 +1,7 @@ | |||
| import { __ } from '@wordpress/i18n'; | |||
|
|
|||
| export const ALLOWED_FORMATS = [ 'core/bold', 'core/italic' ]; | |||
| export const ALLOWED_INNER_BLOCKS = [ 'jetpack/label', 'jetpack/input' ]; | |||
| export const ALLOWED_INNER_BLOCKS = [ 'jetpack/label', 'jetpack/input', 'jetpack/input-range' ]; | |||
There was a problem hiding this comment.
Should this be allowed here or only as inner block of field-range?
|
Overall looking good and file to merge for follow-up iterations. Multiple sliders in single form or multiple forms with sliders in single page are causing trouble: 
When changing max value be smaller than starting value, it feels fine to allow it "on typing", as they might be changing it by removing e.g. zeroes from "100" so it temporarily becomes "1". But when unfocusing and leaving the number mismatched, it might make sense to either:
|
| */ | ||
| function getSliderMinMax( context ) { | ||
| const root = context._rootNode || document; | ||
| const input = root.querySelector( 'input[type="range"]' ); |
There was a problem hiding this comment.
instead of querying the dom could we not pass in the min and max value via the context?
via data-wp-context='{ "foo": "bar" }' see https://developer.wordpress.org/block-editor/reference-guides/interactivity-api/api-reference/#wp-context
Addresses FORMS-66
Proposed changes:
Adds a new slider field block for forms. The goal of this PR is not a final, releasable block. Rather we want to get this to 'good enough' state that we can merge it as a beta block, and then do iterative improvements as needed until it is ready for release.
This is based on designs from this P2: p1HpG7-wSU-p2
Changes include:
This is based on designs in this P2: p1HpG7-wSU-p2
Screenshot: slider field in editor
Screenshot: slider field on frontend
Other information:
Jetpack product discussion
FORMS-66
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Confirm block is hidden by default. Confirm that if you do not have beta blocks enabled, the block is not visible. This is most important, as other improvements to the block itself can be made after the block is merged as a beta block.
Test block.
define( 'JETPACK_BLOCKS_VARIATION', 'beta' );jetpack build packages/formsto get the frontend slider-field.css to be built and loaded in the dist directory. Having the watch command running is not sufficient.