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

Style engine: allow for zero values for CSS properties in the style engine #41561

Merged
merged 3 commits into from
Jun 6, 2022

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Jun 6, 2022

What?

Allow for zero values for CSS properties in the style engine.

Why?

Just in case we come across a style value of '0' , e.g., padding: '0'.

This PR ensures the previous empty() check doesn't fail for '0'.

How?

Create an opinionated, dedicated method so we can use it elsewhere if required.

Testing Instructions

npm run test-unit-php /var/www/html/wp-content/plugins/gutenberg/packages/style-engine/phpunit/class-wp-style-engine-test.php

Create an opinionated, dedicated method so we can use it elsewhere if required.
@ramonjd ramonjd added [Type] Bug An existing feature does not function as intended [Type] Experimental Experimental feature or API. [Package] Style Engine /packages/style-engine labels Jun 6, 2022
@ramonjd ramonjd self-assigned this Jun 6, 2022
@ramonjd ramonjd requested a review from andrewserong June 6, 2022 05:40
Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tests pass, the padding support still works correctly in real world usage, and it sounds good to me to have a specific function for checking the value is valid 👍

Thanks for tidying things up!

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

✅ Tests pass
✅ Padding support works in the editor

Just left a small question though certainly nothing that blocks this.

@@ -165,6 +184,11 @@ protected static function get_slug_from_preset_value( $style_value, $property_ke
*/
protected static function get_classnames( $style_value, $style_definition ) {
$classnames = array();

if ( empty( $style_value ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor question: Is it worth future-proofing this a little in case the classname generation extends beyond presets in future?

This check would still return a false positive for '0'. I'm thinking of things like the Cover blocks opacity classes like has-background-dim-0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting case, thanks for raising it.

There's no requirement for such values here yet, but if we hit the cover block one day it'll be there.

I was just trying to prevent the code doing any further work for now. But there's no harm in removing the check since it wasn't there before. 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ( empty( $style_value ) ) {
if ( empty( $style_value ) && '0' !== $style_value ) {

I was more suggesting that we tweak that check to allow '0' values rather than remove it. Sorry for the confusion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good. I read the comment wrong. In that case it's probably safe to leave as it was until we get to individual blocks in 2025 😄

The fact that the style engine generates classes at all is still a bit contentious, that is, whether it should just stick to CSS.

@ramonjd ramonjd merged commit 11fb168 into trunk Jun 6, 2022
@ramonjd ramonjd deleted the update/style-engine-allow-zero-values branch June 6, 2022 07:44
@github-actions github-actions bot added this to the Gutenberg 13.5 milestone Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Style Engine /packages/style-engine [Type] Bug An existing feature does not function as intended [Type] Experimental Experimental feature or API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants