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

Add spacing presets support to style engine #41990

Merged

Conversation

glendaviesnz
Copy link
Contributor

@glendaviesnz glendaviesnz commented Jun 27, 2022

What?

Adds handling of conversion of spacing preset values to css vars to style engine

Why?

#41527 adds the spacing presets to theme.json, as the first part of implementing standardised design tokens for spacing as detailed in #39371

How?

Extends the existing preset var handling added for the processing of color presets.

Testing Instructions

  • Add the following block markup in the edit code view and save and view in the frontend:
<!-- wp:site-title {"style":{"spacing":{"padding":{"top":"var:preset|spacing|70","right":"var:preset|spacing|50","bottom":"var:preset|spacing|70","left":"var:preset|spacing|50"}},"elements":{"link":{"color":{"text":"var:preset|color|background"}}}},"backgroundColor":"vivid-red"} /-->
  • Check in the frontend that the block markup has converted the var:preset|space|20 values into var(--wp--preset--spacing--20); in the block styles
  • Check that the style engine unit tests run npm run test-unit-php /var/www/html/wp-content/plugins/gutenberg/packages/style-engine/phpunit/class-wp-style-engine-test.php

N.B. The frontend styles markup in this PR is not an indication of the final marking for spacing presets. The final implementation may use utility classes as noted in #39371, but the fact that we are generating these styles dynamically on the frontend means we can modify the output relatively easily as we go.

Screenshots or screencast

Screen Shot 2022-06-28 at 10 24 23 AM

@glendaviesnz glendaviesnz added the CSS Styling Related to editor and front end styles, CSS-specific issues. label Jun 27, 2022
@glendaviesnz glendaviesnz self-assigned this Jun 27, 2022
@glendaviesnz glendaviesnz force-pushed the add/spacing-sizes-to-style-engine branch 2 times, most recently from 3f5fd5b to 1a04521 Compare June 30, 2022 04:46
@glendaviesnz glendaviesnz force-pushed the add/spacing-sizes-to-style-engine branch from 1a04521 to f473509 Compare June 30, 2022 04:56
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.

The PHP implementation is working nicely for me @glendaviesnz, but it looks like the JS implementation needs updating for the individual sides. At first glance, it might only be a couple of lines of code that needs to be changed?

In my local dev environment, I updated this line:

const value: string | undefined = get( boxStyle, [ side ] );

to be the following:

				const value: string | undefined = getCSSVarFromStyleValue(
					get( boxStyle, [ side ] )
				);

And that seemed to do the trick. It might need a test or two added for the JS side of things 🙂

Editor view before (no padding is applied) Editor view after (padding is applied)
image image

Of course, you can always leave implementing the JS-side to a follow-up PR instead if you'd prefer to keep this one just focused on the PHP implementation for now. What do you think?

@glendaviesnz
Copy link
Contributor Author

but it looks like the JS implementation needs updating for the individual sides.

Thanks, I was just focusing on the frontend for this PR, thinking the editor side would be more complicated, but your suggested change works and is simple so have included it.

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.

suggested change works and is simple so have included it

Thanks Glen! This change is testing nicely, and I double-checked that the border color CSS variable output still works correctly too:

image

LGTM! ✨

@glendaviesnz glendaviesnz merged this pull request into add/spacing-sizes-part-1 Jul 1, 2022
@glendaviesnz glendaviesnz deleted the add/spacing-sizes-to-style-engine branch July 1, 2022 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants