Layout: Move layout definitions out of theme.json #50621
Conversation
andrewserong
added
[Type] Bug
andrewserong
changed the title
|
Size Change: +1.05 kB (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in fde9a8d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5261343223
|
|
Just pinging a few folks for early(ish) feedback on the approach in this PR. @youknowriad @oandregal and @tellthemachines, does this seem like a reasonable direction for moving the layout definitions out of I'll be back on Tuesday, but just thought I'd do an early ping while this is still in draft in case anyone has a particular direction in mind for the refactor, and if there are any other considerations to keep in mind (e.g. caching etc). |
lib/block-editor-settings.php
Outdated
| @@ -77,6 +77,8 @@ function gutenberg_get_block_editor_settings( $settings ) { | |||
| // Copied from get_block_editor_settings() at wordpress-develop/block-editor.php. | |||
| $settings['__experimentalFeatures'] = gutenberg_get_global_settings(); | |||
|
|
|||
| $settings['__experimentalFeatures']['layout']['definitions'] = gutenberg_get_layout_definitions(); | |||
There was a problem hiding this comment.
I think this still means plugins can edit these (at least in the frontend). So why don't we avoid the setting at all and just hard code them in the client too. Personally I don't mind the duplication (client/server) but potentially if you don't want duplication you can put them in a json file and have a build step or something to generate the php and the JS?
There was a problem hiding this comment.
Given layout styles are a work in progress (with new layout types such as grid still being worked on), it would be easier for development and less error-prone if these definitions were stored in a single place. A json file might be the best approach if we don't want to add them as block editor settings, though I'm not too concerned about it - the settings being editable by plugins doesn't bother me as much as these definitions being output in generated theme.json files. I assume any plugin author that were to deliberately edit these settings would know and be responsible for what they were doing 😅
There was a problem hiding this comment.
It turned out not to be too bad copying and pasting to a JS file. Very happy for feedback on it, but since we don't change the definitions too frequently, now that I look at it, I don't think it's too much of an issue if we have to update in two places, so long as it's clear that it has to happen in both places.
I've updated to try out a separate JS LAYOUT_DEFINITIONS const.
There was a problem hiding this comment.
That's perfect for me :)
andrewserong
force-pushed
the
try/fix-layout-definitions-and-layout-styles-in-editor-when-theme-styles-are-not-in-use
branch
from
44f8bd7
to
deb1fb7
Compare
lib/block-editor-settings.php
Outdated
| @@ -77,6 +77,8 @@ function gutenberg_get_block_editor_settings( $settings ) { | |||
| // Copied from get_block_editor_settings() at wordpress-develop/block-editor.php. | |||
| $settings['__experimentalFeatures'] = gutenberg_get_global_settings(); | |||
|
|
|||
| $settings['__experimentalFeatures']['layout']['definitions'] = gutenberg_get_layout_definitions(); | |||
There was a problem hiding this comment.
Given layout styles are a work in progress (with new layout types such as grid still being worked on), it would be easier for development and less error-prone if these definitions were stored in a single place. A json file might be the best approach if we don't want to add them as block editor settings, though I'm not too concerned about it - the settings being editable by plugins doesn't bother me as much as these definitions being output in generated theme.json files. I assume any plugin author that were to deliberately edit these settings would know and be responsible for what they were doing 😅
| @@ -596,7 +596,7 @@ describe( 'global styles renderer', () => { | |||
| }; | |||
|
|
|||
| expect( toStyles( Object.freeze( tree ), blockSelectors ) ).toEqual( | |||
| 'body {margin: 0;}' + | |||
| 'body {margin: 0;}body .is-layout-flow > .alignleft { float: left; margin-inline-start: 0; margin-inline-end: 2em; }body .is-layout-flow > .alignright { float: right; margin-inline-start: 2em; margin-inline-end: 0; }body .is-layout-flow > .aligncenter { margin-left: auto !important; margin-right: auto !important; }body .is-layout-constrained > .alignleft { float: left; margin-inline-start: 0; margin-inline-end: 2em; }body .is-layout-constrained > .alignright { float: right; margin-inline-start: 2em; margin-inline-end: 0; }body .is-layout-constrained > .aligncenter { margin-left: auto !important; margin-right: auto !important; }body .is-layout-constrained > :where(:not(.alignleft):not(.alignright):not(.alignfull)) { max-width: var(--wp--style--global--content-size); margin-left: auto !important; margin-right: auto !important; }body .is-layout-constrained > .alignwide { max-width: var(--wp--style--global--wide-size); }body .is-layout-flex { display:flex; }body .is-layout-flex { flex-wrap: wrap; align-items: center; }body .is-layout-flex > * { margin: 0; }body .is-layout-grid { display:grid; }body .is-layout-grid > * { margin: 0; }' + | |||
| '.wp-image img{filter: blur(5px);}' + | |||
There was a problem hiding this comment.
Should the output of toStyles be changing? 🤔
There was a problem hiding this comment.
Yes — previously the test was artificial in that it arbitrarily didn't include layout definitions, but real usage of toStyles would always include layout styles because it would wind up calling getLayoutStyles internally and include the real base theme.json's layout definitions. The updated tests in this PR are now a more accurate representation of the real output.
It is a little jarring when first looking at it, though!
There was a problem hiding this comment.
Ah, got it, thanks for explaining!
|
Thanks for the feedback, folks! I think this is ready for review now. I've pushed some updates, exploring the idea of having a comparable hard-coded list of layout definitions in JS. I think it's slightly less neat, and it also means it'll be slightly more challenging for backporting because the JS isn't indifferent to what's happening on the PHP-side anymore, but I also don't mind the approach overall. When it comes to backporting to Very happy for any feedback on the direction here, and happy to roll back the "layout definitions in JS" side of things if it makes this PR too complex / unwieldy! |
andrewserong
changed the title
| @@ -409,7 +592,7 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) { | |||
| } | |||
|
|
|||
| $class_names = array(); | |||
| $layout_definitions = _wp_array_get( $global_layout_settings, array( 'definitions' ), array() ); | |||
| $layout_definitions = gutenberg_get_layout_definitions(); | |||
There was a problem hiding this comment.
A few lines above, there's some code to query for the data stored in theme.json via gutenberg_get_global_settings. Now that the definitions would be stored elsewhere, does that code need to be updated as well?
There was a problem hiding this comment.
Oh, good catch! Yes, there's no longer a need to check for $global_layout_settings so I've removed a few lines.
We still need the overall $global_settings query for root padding aware alignments, and to grab the root block spacing value (blockGap) if available, so the main query is preserved.
| @@ -1272,7 +1272,7 @@ protected function get_layout_styles( $block_metadata ) { | |||
| $has_block_gap_support = _wp_array_get( $this->theme_json, array( 'settings', 'spacing', 'blockGap' ) ) !== null; | |||
| $has_fallback_gap_support = ! $has_block_gap_support; // This setting isn't useful yet: it exists as a placeholder for a future explicit fallback gap styles support. | |||
| $node = _wp_array_get( $this->theme_json, $block_metadata['path'], array() ); | |||
| $layout_definitions = _wp_array_get( $this->theme_json, array( 'settings', 'layout', 'definitions' ), array() ); | |||
| $layout_definitions = gutenberg_get_layout_definitions(); | |||
There was a problem hiding this comment.
We also need to remove layout.definitions from the VALID_SETTINGS in this class.
There was a problem hiding this comment.
What happened if 3rd parties provided layout.definitions via theme.json? The potential scenarios are: the theme via its theme.json and plugins via the existing filters.
There was a problem hiding this comment.
I found that layout.definitions were merged in core as of 6.1. PR WordPress/wordpress-develop@72380e3 and trac ticket https://core.trac.wordpress.org/ticket/56467 If there wasn't any code to prevent them from being overridden by themes/plugins, we probably need to communicate this via a devnote stating it is a bug that others were able to override the definitions.
There was a problem hiding this comment.
Thanks! I've removed it from layout.definitions. If and when this PR lands, I'm happy to draft a dev note for 6.3 to flag the change 🙂
|
Thanks for doing this. I wonder what parts of the layout styles logic can be removed from the theme.json class to its own standalone class/function/point of reference. Anything that is not processing data coming from any theme.json source should not live in this class. Not for this PR, though. I'm more than happy to help investigate this. |
|
Thanks for taking a look!
I think the main opportunity (for a follow-up, likely beyond 6.3) could be to extract the body of the But in general, I've been wondering if using a similar approach to typography functions in |
There was a problem hiding this comment.
I've tested this with all the core layout blocks in both classic and block themes, checked that Post Content layout is still reflected correctly in the post editor, and that layouts are still working correctly in the site editor too.
The code's looking good! Just a couple minor, very non-blocking comments below.
| * | ||
| * Provides a common definition of slugs, classnames, base styles, and spacing styles for each layout type. | ||
| * When making changes or additions to layout definitions, the corresponding JavaScript definitions should | ||
| * also be updated. |
There was a problem hiding this comment.
Might be nice to add the file path for the JS definitions here?
There was a problem hiding this comment.
I was thinking of adding that in originally, but once we backport the changes, any JS path won't quite make sense from the perspective of wordpress-develop, so I thought I'd leave it off for now. Happy to add it in once we've done the backports, though!
| @@ -706,7 +706,8 @@ describe( 'global styles renderer', () => { | |||
| const style = { spacing: { blockGap: '12px' } }; | |||
|
|
|||
| const layoutStyles = getLayoutStyles( { | |||
| tree: layoutDefinitionsTree, | |||
| layoutDefinitions: | |||
| layoutDefinitionsTree.settings.layout.definitions, | |||
There was a problem hiding this comment.
Would there be any advantage to removing the hardcoded definitions in the test and using the now-hardcoded JS ones? I guess if we leave the test ones we won't have to update the tests every time the real ones change 😅
There was a problem hiding this comment.
I guess if we leave the test ones we won't have to update the tests every time the real ones change 😅
Yeah, for the getLayoutStyles tests, I thought I'd leave it with the hardcoded definitions in the test, since it'll still accept a layoutDefinitions object being passed in, as it saves us a few copy/pastes when we need to update in the future.
I reckon we could probably tidy up these tests a little more in follow-ups, but since we don't need to do it just yet, I thought I'd try to keep the changeset slightly smaller for now.
Happy to update these if anyone feels strongly about it, though!
Wonderful, thank you for the approving review @tellthemachines! I thought I'd just wait overnight to see if there's any final feedback from @youknowriad or @oandregal, and if not, I'll merge this in first thing tomorrow 🙂 |
Code-wise, it looks fine to me. |
There was a problem hiding this comment.
Nice work Andrew! I went through the testing instructions:
✅ Each Group block variation (Group, Row, Stack, Grid) in the post editor, site editor, and site frontend.
--- Default block spacing is output correctly
--- Block is spacing output correctly when set at the individual block level
✅ Adjust global block spacing in the site site editor, adjust block-level spacing in global styles (e.g. block spacing for all social icons blocks), output is consistent in site editor and on site frontend.
✅ Default layout styles are still output correctly in the post editor and site frontend for Classic themes (e.g. the social icons and columns blocks still visually have spacing)
|
Thanks folks! 🙇 I'll merge this in now. |
andrewserong
deleted the
try/fix-layout-definitions-and-layout-styles-in-editor-when-theme-styles-are-not-in-use
branch
andrewserong
added
the
Needs Dev Note
Dev noteLayout definitions removed from core
|
|
Thank you for adding the dev note @tellthemachines! 🙇 |
* Layout: Try moving layout definitions out of theme.json, but retain in block editor settings * Update comment * Add duplicate layout definitions in JS. * Fix some of the JS tests * Update PHP tests * Remove layout definitions from settings * A couple more removals that I previously missed
What?
This is an experimental PR digging into an idea from #50268 (comment) to remove the layout definitions from
theme.json.The approach in this PR is to try to generally retain the same shape and behaviour of the layout definitions, but switch where we retrieve the definitions from. They are now sourced from a PHP function where they're hard-coded, and in JS, they're stored in a
constthat is imported where appropriate.This PR also looks to try to resolve #50011 by calling
getLayoutStylesfrom the post editor.Why?
As raised in #50268 (comment), the layout definitions belong to core and are not intended to be changed or overridden by consumers. It is still useful to have central places where those definitions are stored. This PR initially proposed storing the layout definitions in a single PHP location, but has since evolved to include a corresponding JS constant.
How?
theme.jsonto agutenberg_get_layout_definitionsfunction, and to aconstin JS.gutenberg_get_layout_definitionswherever we need the definitions:layout.phpwhen outputting classnames and rules at the individual block leveltheme.jsonsettingsgetLayoutStylesas a private exportTesting Instructions
This PR refactors both PHP and JS output for layout, so it'd be good to test all of the following to make sure nothing was missed:
Screenshots or screencast