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: Try adding blockGap support #43076

Closed
wants to merge 5 commits into from

Conversation

andrewserong
Copy link
Contributor

What?

🚧 🚧 🚧 🚧 This is just a WIP for now, it does not actually work yet 🚧 🚧 🚧 🚧

The goal with this PR is to explore whether we can migrate the blockGap / block spacing logic over to the style engine, so that we can get the style engine to handle parsing to CSS variables. Hopefully we will also be able to reduce some duplication between layout.php and global styles in the longer term.

Why?

How?

Testing Instructions

Screenshots or screencast

@andrewserong andrewserong self-assigned this Aug 9, 2022
@andrewserong andrewserong added [Package] Style Engine /packages/style-engine [Type] Experimental Experimental feature or API. labels Aug 9, 2022
@@ -162,6 +162,14 @@ class WP_Style_Engine {
'spacing' => '--wp--preset--spacing--$slug',
),
),
'blockGap' => array(
Copy link
Member

@ramonjd ramonjd Aug 9, 2022

Choose a reason for hiding this comment

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

Thanks for looking at this one!

I don't know if it'll help, but I was tried something similar (at least as far as adding a blockGap definition goes) over at https://github.com/WordPress/gutenberg/pull/40955/files#diff-69616e5e4ff1a06362db0b7165c330b00614d4ea0d3a28c80003c0860ab8b93fR176

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, thank you! I'm sure it will — I was mostly hacking around pretty quickly on this yesterday to see how we may or may not be able to slot things in, and put up the PR to save my work. I have no idea if the approach will work / be viable, but was curious to have a poke around 😄

With this PR I'm hoping to see whether on balance, it's worth attempting to move some of the Layout logic (specifically blockGap) over to the style engine, or if it's just easier to keep it as part of the Layout logic, and then duplicate the parsing of CSS variables.

The end goal is so that once the presets-based UI for spacing is ready, we are then able to roll it out to block gap/spacing in addition to padding and margins. 🤞

@andrewserong
Copy link
Contributor Author

I've added a commit (4f69c29) that sorta-kinda gets this working technically. Just thought I'd save my progress with the messy commit before giving this a rebase. Here's visual evidence that the gap values are being output for Group and Social Icons blocks:

image

@andrewserong andrewserong force-pushed the try/style-engine-add-block-gap-support branch from 4f69c29 to 1349f2f Compare August 16, 2022 05:00
@andrewserong
Copy link
Contributor Author

Just jotting down a few to-do list items while they're top of mind:

  • Get split axial values working
  • Get working in JS implementations (block level in post editor, global styles)
  • The returning of declarations vs rules is now inconsistent — when should it be declarations vs when should it be rules, and is this PR adding too much complexity to the output?
Some test markup:
<!-- wp:group {"style":{"spacing":{"blockGap":"100px"}}} -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>A paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>Another paragraph</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

<!-- wp:social-links {"style":{"spacing":{"blockGap":{"top":"200px","left":"200px"}}}} -->
<ul class="wp-block-social-links"><!-- wp:social-link {"url":"wordpress.org","service":"wordpress"} /-->

<!-- wp:social-link {"url":"wordpress.org","service":"wordpress"} /--></ul>
<!-- /wp:social-links -->

<!-- wp:group {"style":{"border":{"width":"9px","radius":"14px"},"spacing":{"padding":{"top":"var:preset|spacing|40","right":"var:preset|spacing|40","bottom":"var:preset|spacing|40","left":"var:preset|spacing|40"}}},"borderColor":"luminous-vivid-orange"} -->
<div class="wp-block-group has-border-color has-luminous-vivid-orange-border-color" style="border-width:9px;border-radius:14px;padding-top:var(--wp--preset--spacing--40);padding-right:var(--wp--preset--spacing--40);padding-bottom:var(--wp--preset--spacing--40);padding-left:var(--wp--preset--spacing--40)"></div>
<!-- /wp:group -->

<!-- wp:post-title {"style":{"spacing":{"margin":{"top":"21px","right":"21px","bottom":"21px","left":"21px"}},"typography":{"textTransform":"uppercase","fontStyle":"italic","fontWeight":"800"},"border":{"width":"15px","radius":"26px"}},"fontSize":"x-large"} /-->

<!-- wp:group {"style":{"spacing":{"blockGap":"var:preset|spacing|40"}}} -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>A paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>Another paragraph</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

<!-- wp:social-links {"style":{"spacing":{"blockGap":{"top":"var:preset|spacing|40","left":"var:preset|spacing|40"}}}} -->
<ul class="wp-block-social-links"><!-- wp:social-link {"url":"wordpress.org","service":"wordpress"} /-->

<!-- wp:social-link {"url":"wordpress.org","service":"wordpress"} /--></ul>
<!-- /wp:social-links -->

<!-- wp:group {"style":{"spacing":{"blockGap":"var:preset|spacing|40"}},"layout":{"contentSize":"261px"}} -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>A paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>Another paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:social-links {"style":{"spacing":{"blockGap":{"top":"4px","left":"18px"}}}} -->
<ul class="wp-block-social-links"><!-- wp:social-link {"url":"wordpress.org","service":"wordpress"} /-->

<!-- wp:social-link {"url":"wordpress.org","service":"wordpress"} /-->

<!-- wp:social-link {"url":"wordpress.org","service":"wordpress"} /-->

<!-- wp:social-link {"url":"wordpress.org","service":"wordpress"} /-->

<!-- wp:social-link {"url":"wordpress.org","service":"wordpress"} /-->

<!-- wp:social-link {"url":"wordpress.org","service":"wordpress"} /-->

<!-- wp:social-link {"url":"wordpress.org","service":"wordpress"} /-->

<!-- wp:social-link {"url":"wordpress.org","service":"wordpress"} /--></ul>
<!-- /wp:social-links --></div>
<!-- /wp:group -->

@andrewserong
Copy link
Contributor Author

After spending a bit of time trying to get this to work properly, I think it'll add unneeded complexity at this stage. I've opened up an alternate PR in #43296 to add support for spacing presets to the current way we output blockGap. This has a few benefits over this approach:

  • Support in the Theme JSON class is quite simple to implement
  • Support in JS was also quite simple, in that we could re-use an existing function to do the parsing
  • The duplication in layout.php is pretty minimal

For these reasons, and that the feature freeze for 6.1 is coming fairly soon, I think it'd probably be better to park this PR (exploring moving blockGap output to the style engine) for now, and we can always revisit as part of 6.2 if we think blockGap has a better home in the style engine.

I'll close this for now, but happy to re-open the PR again if anyone feels strongly about it!

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] Experimental Experimental feature or API.
Projects
Status: 🗄 Closed / not merged
Development

Successfully merging this pull request may close these issues.

2 participants