Skip to content

Add spacing presets support to style engine #41990

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 1, 2022
Merged

Add spacing presets support to style engine #41990

merged 2 commits into from
Jul 1, 2022

Conversation

glendaviesnz
Copy link
Contributor

@glendaviesnz glendaviesnz commented Jun 27, 2022 •
edited
Loading

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 mentioned this pull request Jun 27, 2022
Closed
@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
andrewserong
andrewserong reviewed Jul 1, 2022
View reviewed changes
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:

gutenberg/packages/style-engine/src/styles/utils.ts

Line 84 in d265f30

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.

andrewserong
andrewserong approved these changes Jul 1, 2022
View reviewed changes
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
@glendaviesnz glendaviesnz mentioned this pull request Jul 5, 2022
Open
18 tasks
@andrewserong andrewserong mentioned this pull request Jul 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewserong andrewserong andrewserong approved these changes

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey

Assignees

@glendaviesnz glendaviesnz

Labels
CSS Styling Related to editor and front end styles, CSS-specific issues.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@glendaviesnz @andrewserong