-
Notifications
You must be signed in to change notification settings - Fork 814
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
Forms: add number input #40962
Forms: add number input #40962
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 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? 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. |
7b5380d
to
975a521
Compare
e7be37d
to
2eac76d
Compare
Code Coverage SummaryCoverage changed in 5 files.
Full summary · PHP report · JS report Add label
I don't care about code coverage for this PR
|
@@ -52,7 +52,8 @@ const JetpackField = props => { | |||
onChange={ e => setAttributes( { placeholder: e.target.value } ) } | |||
style={ fieldStyle } | |||
type={ type } | |||
value={ placeholder } | |||
value="" | |||
placeholder={ placeholder } |
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.
c1d6293
to
93c3317
Compare
Nice work. We are getting close. I think we still need to fix up some of the styling and make sure it looks more consistent across themes and from styles. The numbers input field looks like (across the different form styles) There are a couple of other ones that I came across with missing styling. |
icon: renderMaterialIcon( | ||
<Path | ||
fill={ getIconColor() } | ||
d="M12 7H4V8.5H12V7ZM19.75 17.25V10.75H4.25V17.25H19.75ZM5.75 15.75V12.25H18.25V15.75H5.75Z" |
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.
on a follow up PR we should probably design a different icon?
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! :-)
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.
looking good!
Yeah I just double checked this and it it works well in Archo theme for me now 🥳 What kinds of numbers should we allow? Do we allow only limit it to integers? How about negative numbers? IMHO we should allows be as liberal as possible and allow any type of number by default. ( Since most folk might not think about thus much) but folks that do might want to have very specific requirements. If you add step="any" it will allow for floating point (decimals) as well. |
👍 let's merge and do follow ups? |
I agree that's up to the site creator. :-) There will be min/max/step configuration which they can use to limit it, but not in this PR. |
I wanna hold off from merging until placeholder question is solved (#41712), or otherwise I'll disable placeholder UI in this PR for the Number field and add it back later. |
455280c
to
8aea8e0
Compare
I disabled placeholder input in the sidebar for number field for now — we'll add it back with #41712 |
Issue #40961
Proposed changes:
Disables setting placeholders via sidebar, allowing setting them only in the editor (and thus only be number values for now) — PR for general improvement for all fields in #41712
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
Number needs to appear in several places after submitting:
Confirmation message

Email

Inbox

Export
