-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Backport Layout block support refactor part 2 #3254
Backport Layout block support refactor part 2 #3254
Conversation
@tellthemachines I believe this one is working correctly — it just depends on #3218 in order for the generated styles to be output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for handling this PR @andrewserong! I've left some thoughts below.
*/ | ||
public function data_wp_get_layout_style() { | ||
return array( | ||
'should_return_empty_value_with_no_args' => array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the wording for each of these, thanks!
Thanks for all the feedback @costdev! I believe I've implemented it all, so this should be ready for another look now 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good! In testing all the correct styles and classnames are output on the front end.
I also tested with the #3154 patch applied, and everything worked correctly in the post editor (minus some PHP warnings that are not related to this PR). I wasn't able to test the site editor because it doesn't load at all once #3154 is applied. It's likely that those packages are dependent on another PHP backport 🤔
@@ -1,5 +1,5 @@ | |||
|
|||
<div class="wp-container-1 wp-block-columns has-3-columns"> | |||
<div class="is-layout-flex wp-container-1 wp-block-columns has-3-columns"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this keep wp-container-1
because it has non-default flex settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, columns set nowrap
so it has a non-default setting from the perspective of the flex Layout type. Thanks for double-checking!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @andrewserong! Just one suggestion left from me then LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Thanks for all the work on this @andrewserong!
Co-authored-by: Colin Stewart <[email protected]>
18118b2
to
d64d61d
Compare
I've given this a rebase, and confirmed that it's still working correctly on top of |
I'm self-assigning for review and commit. |
Test ReportSteps to Test
Expected Results
Environment
Actual Results
Supplemental Artifacts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @andrewserong LGTM. Left one minor change.
Co-authored-by: Mukesh Panchal <[email protected]>
For this backport, I'll be reverting the DocBlock changes for
Instead, I'll do a follow-up coding standards commit to improve the DocBlock including updating the descriptions for the optional parameters (e.g. adding the word |
* Reverts changes not found in Gutenberg PR 40875 * Updates changes from Gutenberg PR 40875 for coding standards WordPress/gutenberg#40875
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Changes in this PR meet Core's coding standards ✅
- Other changes not found in the original GB PR have been removed (follow-up commit coming) ✅
- Per testing instructions, confirmed works ✅
- PHP 8.1 and 8.2 tests do not show any additional deprecation issues ✅
There are additional tweaks that could be done. However, to retain a more pure backport, those will be addressed in follow-up patch(es)/ commit(s).
This is ready for commit. Prepping commit now.
Committed via changeset https://core.trac.wordpress.org/changeset/54274. Thank you everyone for your contributions! A follow-up commit is coming to address other changes that were not part of the original GB PR (such as DocBlock improvements). |
DocBlock improvements suggestioned in this PR were committed via changeset https://core.trac.wordpress.org/changeset/54275. |
This PR is a follow-up to #3205 and backports the remaining parts of the layout block support.
Note: the updates to the server test fixtures are all expected changes — the updates to the Layout support are that we now also output semantic classnames for the layout types
(e.g.
is-layout-flowor
is-layout-flex). Also, the
wp-container-xxx` classname will no longer be output for many blocks that use the default layout settings.To-do
Backport testsEnsure it's working on top of Backport script loader: enqueue stored block supports styles #3218To confirm that it's working, you can manually copy the changes from #3218 on top of this PR, and in the rendered view of TwentyTwentyTwo theme, the space between styling of the row block should be output correctly as in the screenshot below (note the highlighted layout classnames that have been added there, too
is-layout-flex
, etc):Trac ticket: https://core.trac.wordpress.org/ticket/56467
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.