Skip to content
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

Conversation

andrewserong
Copy link
Contributor

@andrewserong andrewserong commented Jun 7, 2021

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 or 1.5rem, this change proposes adding an allowDecimal prop to the NumberControl, defaulted to false 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 or rem 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:

npm run test-unit -- packages/components/src/number-control/test/index.js
npm run test-unit -- packages/components/src/utils/test/math.js

Screenshots

number-control-sml

Types of changes

Enhancement (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers. (tested via keyboard)
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Jun 11, 2021

Works for me, but just wondering if we want to limit number of decimal places?, eg. I was able to add:
{"top":"3.14159265359em","right":"3.14159265359em","bottom":"3.14159265359em","left":"3.14159265359em"}
and given the size of the box it is then hard to edit anything past 2 decimal places, and probably unlikely that anyone wants to go past 1 decimal place in reality.

@andrewserong
Copy link
Contributor Author

andrewserong commented Jun 11, 2021

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, 1.618rem). There's a bunch of different scales at: https://type-scale.com/ and https://www.layoutgridcalculator.com/typographic-scale/

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 em or rem values to get things lining up nicely (e.g. margins or padding proportional to font size, using one of those mathematical scales). But I agree, it makes sense to add some kind of limit to avoid very long values!

@aaronrobertshaw
Copy link
Contributor

Works for me, but just wondering if we want to limit number of decimal places?

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.

@andrewserong
Copy link
Contributor Author

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.

@andrewserong
Copy link
Contributor Author

Thanks for the feedback, folks! I've updated this to round to 5 decimal places. I've seen em and rem values out in the wild up to 4 decimal places, so this gives a tiny bit of extra wriggle room so that users hopefully won't even notice that the custom value they're entering would be rounded. Since I imagine these longer values (past a couple of decimal places) would be pretty rare, I don't think it's worth trying to update the UI in this PR.

I've moved the clamp logic over to the utils/maths.js module and added another function there, which removes the additional lodash import. Since lodash (and clamp) is already in that module, I thought I'd leave looking at removing lodash from these maths functions to another time. Superficially clamp doesn't seem all that complicated, but I thought it best to avoid getting tripped up on the existing behaviour.

Let me know if you'd like any other changes, or can think of a better name for this feature/prop than allowDecimal. Thanks! 🙇

@andrewserong
Copy link
Contributor Author

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.

@andrewserong andrewserong force-pushed the update/number-control-with-option-to-skip-rounding branch from c1c9af3 to 2145bbf Compare June 17, 2021 07:22
@andrewserong
Copy link
Contributor Author

Rebased, fixed a linting issue, and added allowDecimal to the vertical/horizontal controls added in: #32610.

@andrewserong andrewserong self-assigned this Jun 17, 2021
@aristath
Copy link
Member

I just saw this PR now. An alternative to this is #32692 which takes a different approach:

  • Adds a step to all units
  • Makes the number control respect the defined step 'cause from what I could tell that was the main issue... We can define a step in the control, but roundClamp was not respecting that.

@andrewserong
Copy link
Contributor Author

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?

@aristath
Copy link
Member

Hmmm I'm trying to figure out how much accuracy is reasonable vs overkill 🤔

If we have some reasonable default step values for all units, are ultra-specific values necessary?
Example: Assuming a standard base font-size of 16px, the difference between 1.333em and 1.33em is 0.048px. If the base font-size is 32px, then the difference is 0.096px so in both cases the values are so minuscule that become un-renderable by the browser. To get to half a pixel, the base font-size would have to be 167px, which is not impossible, but at that size, half a pixel is just meaningless 😅
Isn't rounding to 2 decimals (step:0.01) enough for em/rem/ch/ex units? Is there a realistic scenario where users would need more accuracy than that?

@andrewserong
Copy link
Contributor Author

andrewserong commented Jun 17, 2021

Thanks for indulging me @aristath!

If we have some reasonable default step values for all units, are ultra-specific values necessary?

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 rem units are used to get closer to rounded pixel values. So perhaps 1.333 isn't the best example, as you say it ends up resolving to sub-pixel values where I'd be hard pressed to tell the difference on a HiDPI screen! Possibly the Major Third scale has better examples with 1.563rem to get close to 25px if the base font size is 16px? But even then, personally I'd think 1.56rem probably gets you close enough 🤔

Isn't rounding to 2 decimals (step:0.01) enough for em/rem/ch/ex units? Is there a realistic scenario where users would need more accuracy than that?

For my own purposes, I'd be perfectly happy with just being able to use 1.25em and 1.5em, which I expect would cover most real-world users 🙂

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!)

@aristath
Copy link
Member

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 1.333 instead of 1.33 in the input-field itself, regardless of whether that makes a difference on their site or not. 1.825 feels different than 1.83 even though it's practically the same.

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.
Now that step works as it should, maybe we could use it? 🤔
What if instead of tweaking the number inputs, we tweak unit-control? If the user types a value, we could check the decimals of the entered value and change the step accordingly. The default step values defined in the units themselves can be used as sane defaults, and as the users enters their value, if they need better accuracy they'll get it. Of course we'll need to put a limit and say no more than 3-4 decimals for sanity's sake (I don't want to see people using the first 100 digits of golden-ratio or π as a value, 3-4 digits are more than enough 🤣)

@andrewserong
Copy link
Contributor Author

Well, actually not everyone but the majority. Some will always complain no matter the implementation

Ha! Very true 🙂

What if instead of tweaking the number inputs, we tweak unit-control? If the user types a value, we could check the decimals of the entered value and change the step accordingly.

Oh, I actually really like that idea. I'm just finishing up my week, but happy to explore that next week!

I don't want to see people using the first 100 digits of golden-ratio or π as a value, 3-4 digits are more than enough

Agreed! Flexibility is great... up to a point 😄

Thanks again for the discussion!

@aaronrobertshaw
Copy link
Contributor

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.

@aristath
Copy link
Member

Does the same step for each unit make sense for font size, letter spacing, margins, border radius etc?

Yep! The default steps for units make sense in all cases I could think of 👍

Keeping the ability to supply an explicit value or adapt based on user supplied input sounds like a good safety net to me.

Agreed

@andrewserong
Copy link
Contributor Author

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 🙂

@andrewserong
Copy link
Contributor Author

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.

@andrewserong
Copy link
Contributor Author

I'll close this PR out now, as I've started exploring the approach of updating the step value based on the real value within the UnitControl component. It isn't quite working right (as it doesn't seem like we can trigger a re-render of the parent UnitControl component from within the unitControlStateReducer), but I'll continue exploring over in #33156 when I have a bit of time. Happy for any feedback, of course!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants