-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Number Control: Add option to skip rounding of value for BoxControl #32481
Number Control: Add option to skip rounding of value for BoxControl #32481
Conversation
Works for me, but just wondering if we want to limit number of decimal places?, eg. I was able to add: |
Thanks for testing, Glen! With typography at least, I often see in the wild up to 3 decimal places when sites use scales like the Golden Ratio (say, With this PR I was mostly thinking of folks designing block patterns (or users inserting patterns) where the design of the pattern might have those ratios in mind and require a bit of finesse with the |
I'd like to second adding a limit to the number of decimal places. Previously, I have encountered some block validation errors in the past arising due to a difference in how PHP and JS rounded numbers. In the earlier case, it was relating to a percentage flex-basis style to be applied inline. When the content was saved it still was passed through PHP attribute filtering etc which caused it to get PHP's rounded value which was limited to 12 decimal places. When reloading the editor, JS allowed up to 15 decimal places so that no longer matched the saved content resulting in a block validation error. Applying a limit so that values set via the number control can't run into the above issue, would definitely make it all a little robust. |
Great point, thanks for the input Aaron! I'll have a play with limiting it to a more sensible length. I'm also curious if we can safely remove lodash from the equation, too. |
Thanks for the feedback, folks! I've updated this to round to 5 decimal places. I've seen I've moved the clamp logic over to the Let me know if you'd like any other changes, or can think of a better name for this feature/prop than |
Now that #32610 has landed, this PR will need a little bit of additional work to line up nicely. I'll take a look later on today. |
c1c9af3
to
2145bbf
Compare
Rebased, fixed a linting issue, and added |
I just saw this PR now. An alternative to this is #32692 which takes a different approach:
|
Thanks for the heads-up @aristath! Nice, I actually wonder if both approaches could work together. Adding in the step in that PR will also improve incrementing up / down behaviour and should make it feel more natural. Then, with this PR if a user needs a more specific value (e.g. 1.25rem or 1.333) they can still enter it manually via keyboard. Of course this could also be revealing my personal fondness for oddly specific numbers! 😄 What do you think? |
Hmmm I'm trying to figure out how much accuracy is reasonable vs overkill 🤔 If we have some reasonable default |
Thanks for indulging me @aristath!
Quite possibly not! But the way that I've been thinking of it, is that step values work really nicely for incrementing up and down, and while dragging the input field, but that we might want the value for the step to sometimes be broader than what a user might need for a specific value. And a more specific step value might effectively slow the drag speed of the mouse when a user is changing the value, so coming up with a step value that works for both could be a subtle process. When it comes to these ultra-specific values, the main examples I've seen are when designers are using a type scale system like in https://type-scale.com/ where
For my own purposes, I'd be perfectly happy with just being able to use But my main thinking is that if there are designers (I'm mostly thinking long-term of people designing patterns for other users, rather than the majority of end users) who like to use tools like Type Scale or https://www.modularscale.com/, then it'd be good for the field to support their specific values, rather than round custom values unexpectedly. This is most likely a tiny use case, but I'm more thinking of it from the perspective of "as a designer, I want to be able to copy + paste this value I got from a type scale website or from Figma", rather than the highly specific value really being all that useful in practice. But also, I'm possibly overthinking all this! 😄 (Also, don't let this discussion stop you from merging in your PR if it fixes the issue in the short-term!) |
I can understand that, and in my own theme, I also use type-scales. In practice, rounded and non-rounded values are pretty much the same thing, but I can understand how some would want to visually see I rather like this PR, and would love it if we could "marry" the 2 implementations somehow and come up with a solution that would benefit everyone. Well, actually not everyone but the majority. Some will always complain no matter the implementation 😆 #32692 got merged today, but I wonder how we could make both ideas work. |
Ha! Very true 🙂
Oh, I actually really like that idea. I'm just finishing up my week, but happy to explore that next week!
Agreed! Flexibility is great... up to a point 😄 Thanks again for the discussion! |
There's another scenario that might be worth raising here although I'm not sure it changes much in the discussion so far. These controls can be added via block support. We might not really know ahead of time what degree of accuracy is required or what would make a sane default for a step value in each instance. Does the same step for each unit make sense for font size, letter spacing, margins, border radius etc? I suspect that in general a per unit default would suffice. Keeping the ability to supply an explicit value or adapt based on user supplied input sounds like a good safety net to me. |
Yep! The default steps for units make sense in all cases I could think of 👍
Agreed |
Thanks for the extra input, folks! I've switched over to a bit of other work this week, but hope to get back to this before the week's end 🙂 |
Well, that didn't quite work out the way I'd planned this week. 🤞 for next week! We'll have wrapped up another chunk of work, so I should be able to get back to this and related spacing work next week. |
I'll close this PR out now, as I've started exploring the approach of updating the |
Description
Related to: #31964
Currently, the NumberControl component clamps and rounds the input value when the value is committed (either pressing enter or tabbing out of the field), which will round the value to the nearest step value. However, in controls like the BoxControl, we need to allow the user to commit values with decimal places, which is not currently supported. To enable, for example, padding values of
1.25em
or1.5rem
, this change proposes adding anallowDecimal
prop to the NumberControl, defaulted tofalse
so that we can switch off the rounding when we'd like to.My working assumption here is that while we might want to use step values for pressing up and down in an input field, or for drag behaviour, we might still want to allow users to set specific values within those step ranges. For example, to allow folks to design or use block patterns that might need to conform to a highly specific scale ratio (e.g. like some of those from: https://type-scale.com/)
How has this been tested?
Add a Group block to a post or page. Under Spacing, select a non-px unit, e.g.
em
orrem
and manually enter a value with a decimal place, e.g.1.5em
. When you press enter or tab out of the field, the value should stick, where it did not before.Run tests locally:
Screenshots
Types of changes
Enhancement (non-breaking change which adds functionality)
Checklist:
*.native.js
files for terms that need renaming or removal).