Formatting: add new rounding css number formatting function #57796
Conversation
ramonjd
added
[Status] In Progress
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.5/formatting.php ❔ phpunit/formatting-test.php ❔ lib/load.php |
added css number rounding function
Using `number_format` to ensure that there's a dot decimal. That way we can explode the string. Added unit tests
ramonjd
force-pushed
the
add/css-number-formatting-function
branch
from
7057ef4 to
571c7a6
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @g12i. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
This is great, and I'm glad to see WordPress get a new vocabulary word with will help developers more easily write reliable CSS.
Still, I think the role of the rounding and decimal_places is unclear at best, damaging at worst.
I've been looking to better understand how to ensure this behaves in an expected way. There are times I can imagine people sending hand-crafted values to it, which they want to come across in the CSS as written; and there are times I can imagine this gets the result of some computation, like division, leaving arbitrary precision that someone wants to cut at some reasonable value (although why I don't understand since the browser performs the appropriate rounding when styling).
It seems like significant digits far more reliably would capture the intent of the developer sending values here. That is, I would expect 0.000057 to come across verbatim, while I might want to truncate 0.00005700000000000000313811476804204403379117138683795928955078125 (which is the actual-stored-value of 0.000057 because of floating-point rounding error) to 0.000057. I'm having trouble finding built-in functions that does exactly as I expect, but we could always hand-code an algorithm that does.
| * Optional. An array of options. Default empty array. | ||
| * } | ||
| * @return string A rounded CSS number. | ||
| */ |
There was a problem hiding this comment.
there are some things a linter will reject when merging into Core:
- this needs to have a newline-separated summary at the top. it can be one or two lines, but has to be separated from the description by a blank comment line.
- add
@since 6.7.0before the param tags but after everything else, separate with blank lines. - the link to Mozilla is great, but it might be better below the example, above the
@since, and starting with@see. you might consider linking to the specification itself rather than the MDN page describing it. - for the example code you can follow the following guide
/**
* Something
*
* Example:
*
* $padding_rounded = …
* echo "padding: {$padding_rounded}px;"Note the Example:, blank line, and addition four-space indent on each example line.
| * See https://developer.mozilla.org/en-US/docs/Web/CSS/number | ||
| * Normalizes LM_NUMERIC locales with non-dot decimal point, e.g., "1,234.56" to "1234.56". | ||
| * The above is relevant to PHP < 8, whose float to string casting is locale-dependent. | ||
| * See https://php.watch/versions/8.0/float-to-string-locale-independent |
There was a problem hiding this comment.
The bit about LC_NUMERIC (note the typo here as well) may be more incidental to the original bug than the semantic of this new method. Perhaps consider making this a separated paragraph and focusing more on the second sentence, which I think better conveys the motivation for this function.
/**
* Formats an integer or float value as a CSS number.
*
* CSS numbers must not be localized, but PHP < 8 performs this localization
* by default when casting numbers to string. This function ensures that the
* CSS value for a number is appropriate according to the specification.
*
* Example:
*
* "25.4" === wp_round_css_value( 20.3566 + 5, array( 'decimal_places' => 1 ) );
*
* @see https://drafts.csswg.org/css-values/#numbers
*
* @since 6.7.0
*
* @param int|float $value Numeric value to format as a CSS number.| * $padding_rounded = gutenberg_round_css_value( 20.3566 + 5, array( 'decimal_places' => 1 ) ); | ||
| * echo "padding: {$padding_rounded}px;" // "padding: 25.4px;" | ||
| * | ||
| * @param int|string|float $value A CSS <number> data type. |
There was a problem hiding this comment.
is the string input necessary here? it would seem to pollute an otherwise clear interface. for example, what if someone sends 20px or 2e3f? These turn into 20 and 2000, respectively. It becomes much more complicated in the string case to know if something is legitimate or not, but if we require the caller to only send int|float then there's no wiggle room. If they accidentally send a string it's on them to convert it first, when they have the context to know how to perform the conversion.
There was a problem hiding this comment.
Good points, I agree it would reduce complexity.
I think I accepted strings mainly out of convenience, so folks consuming theme.json values wouldn't have to coerce before using this function.
But I added a note about the CSS , so on a bizarre subconscious level, I was halfway there. 😄
| * $padding_rounded = gutenberg_round_css_value( 20.3566 + 5, array( 'decimal_places' => 1 ) ); | ||
| * echo "padding: {$padding_rounded}px;" // "padding: 25.4px;" | ||
| * | ||
| * @param int|string|float $value A CSS <number> data type. |
There was a problem hiding this comment.
side note: if these were already CSS <number> values then we wouldn't need this function. these _need to become CSS ` values, so this is more about the output than the input.
| if ( is_numeric( $value ) && is_float( floatval( $value ) ) ) { | ||
| $defaults = array( | ||
| 'decimal_places' => 3, | ||
| ); |
There was a problem hiding this comment.
do we have good evidence to know that decimal_places is an appropriate option? is this the preferred method in use by existing code? how do existing casts handle it? do they at all?
again, I would like to challenge the assumption that modifying this for aesthetic purposes is helpful. things that are changed in order to look good often raise surprising bugs.
there are options here:
- decimal places focuses on the granularity of the measure
- significant digits focuses on the impact of the measure
- no truncation focus on faithfully representing the value provided to the function
if we leave this out then literally nothing bad will happen in the browser. by introducing it we impose a new specification on top of CSS that it doesn't provide. as a helper it can be nice, but what is our point in wanting to eagerly remove the information from the number?
There was a problem hiding this comment.
I think consumer could take responsibility for the rounding - it was there as a matter of convenience, and, admittedly a bias towards my typography use case.
I'll take your advice on this and remove with further testing.
There was a problem hiding this comment.
Oh, I just remembered something else:
number_format always wants to do rounding, and it was so useful here I coded around its requirements. 🤷🏻
By "useful" I mean it reliably formatted the number using a decimal point without the use of setlocale()
There was a problem hiding this comment.
it reliably rounds to a given decimal digit count, but not to significant digits. did you try number_format( 2.3e15, 3, '.', '' )? that is 2300000000000000.000
this is why I think significant figures is a much better estimate if we want to modify the numeric information.
| $split_value = explode( '.', $value ); | ||
| $is_float = isset( $split_value[1] ) && intval( $split_value[1] ) > 0; | ||
|
|
||
| return $is_float ? "{$split_value[0]}." . rtrim( $split_value[1], '0' ) : "{$split_value[0]}"; |
There was a problem hiding this comment.
here it seems like it could simplify things to fast-track integer inputs. does PHP ever add commas to int casts?
// if PHP never adds commas to int values
if ( is_int( $value ) ) {
return (string) $value;
}
// if PHP ever adds commas to int values…
if ( is_int( $value ) ) {
return number_format( $value, 0, '.', '' );
}There was a problem hiding this comment.
Looks like it. Shortcircuiting for ints is 👍🏻
<?php
// PHP 7.4
setlocale( LC_ALL, 'de_DE.UTF-8' );
echo 20000.2342423424324; // 20000,234242342
$num = 20000.324234234;
echo $num; // 20000,324234234
$px = $num . "px";
echo $px; // 20000,324234234px
There was a problem hiding this comment.
thanks for confirming. according to the docs this seems like it should work too
return number_format( $value, 0, '.', '' );| 'decimal_places' => 3, | ||
| ); | ||
|
|
||
| $options = wp_parse_args( $options, $defaults ); |
There was a problem hiding this comment.
if we think only ever need this one parameter (or if we even need this single parameter at all) then we might eliminate this complicated call with some surprising behaviors and instead rely on something more direct.
do we expect people to call wp_round_css_value( $value, '&decimal_places=5' )? if not, this performs stronger validation.
$decimal_places = $options['decimal_places'] ?? null;
if ( ! is_int( $decimal_places ) ) {
$decimal_places = 3;
}but taking out the options array and leaving it as $decimal_places in the function argument (which, by the way, will integrate much better with IDE's wanting to provide inline documentation and autocomplete), then this suffices.
$decimal_places = is_int( $decimal_places ) ? $decimal_places : 3;but again I wonder if this parameter is even helpful given that it removes information from the number provided, leading to situations where it's difficult if not possible to get WordPress to spit out the number a developer wants for their pixel-perfect alignment. I'm thinking specifically of situations where you return some number from a filter but other code prints out the returned value to CSS, but applies some arbitrary truncation to the number, and there's no ability to extend that.
There was a problem hiding this comment.
it seems like it breaks certain classes of numbers with small values.
"0" === gutenberg_round_css_value( 0.000057 );
"0.001" === gutenberg_round_css_value( 0.00057 );
"0.006" === gutenberg_round_css_value( 0.0057 );All of these seem wrong to me.
| @@ -152,6 +152,9 @@ function gutenberg_is_experiment_enabled( $name ) { | |||
| require __DIR__ . '/compat/wordpress-6.6/option.php'; | |||
| require __DIR__ . '/compat/wordpress-6.6/post.php'; | |||
|
|
|||
| // WordPress 6.7 compat. | |||
| require __DIR__ . '/compat/wordpress-6.7/formatting.php'; | |||
There was a problem hiding this comment.
this isn't so much about formatting as it is about CSS. perhaps a name related to CSS would clarify, such as css-utilities.php or css-helpers.php
there's a style-engine directory in Core I could see this going into, but so far we don't have any equivalents to what you're doing. I support something like css-helpers.php but I don't know if others will like that for a single function.
| * } | ||
| * @return string A rounded CSS number. | ||
| */ | ||
| function gutenberg_round_css_value( $value, $options = array() ) { |
There was a problem hiding this comment.
what about gutenberg_rounded_css_value() to focus more on the output than the process?
alternatively, just gutenberg_format_css_number() or gutenberg_css_number() or gutenberg_to_css_number() all speak more directly to the semantic this is attempting to provide: take a PHP number and return a CSS number.
| $options = wp_parse_args( $options, $defaults ); | ||
| $value = number_format( $value, $options['decimal_places'], '.', '' ); | ||
| $split_value = explode( '.', $value ); | ||
| $is_float = isset( $split_value[1] ) && intval( $split_value[1] ) > 0; |
There was a problem hiding this comment.
it's probably important here to detect is_nan( $value ) and handle that separately
|
Dennis, I love your reviews. Thanks again. While I'm still unconvinced of the need for this right now, I'd like to make those changes when I get time, and keep testing so it's on hand for when the customers come knocking. |
|
Closing for now. I'll come back later if this feature is required and time permits. |
What?
Partly resolves #57607
If this PR is merged, it can be used in a refactor of:
This time, create a function that can be used to create rounded CSS number strings from anywhere!.
It also outputs valid CSS values where locales use commas as decimal separators.
Why?
In PHP version < 8, PHP uses the current locale for float to string conversions.
That means
1.333 + "px", for example, will be converted to1,333pxfor some locales, e.g.,pl_PL,fr_FRetcPHP 8 does not do this.
See: https://php.watch/versions/8.0/float-to-string-locale-independent
This is so we can output valid CSS values where locales use commas as decimal separators.
Questions
Is there enough test coverage? Do the PHP comments make sense? Can the function be improved give its stated purpose?
Testing Instructions
Unit
npm run test:unit:php:base -- --filter Gutenberg_Formatting_TestManual
To test manually, we need to set up a server with < PHP 8 and also with a non-en LC_NUMERIC locale.
See the below diff on the changes to wp-env/docker and also setting the local via
setlocale.Finally, use the new function
gutenberg_round_css_valuewhen outputting fluid font sizes.Example test environment
I'm using TT4 to test. Once you apply the changes in the example diff, run the following commands:
npm run wp-env destroy # Hit `y` to destroy your local docker containers npm run wp-env startWith the environment set up, the fluid font sizes should contain expected decimal values:
Before