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

Backport Layout block support refactor part 2 #3254

Conversation

andrewserong
Copy link

@andrewserong andrewserong commented Sep 15, 2022

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-floworis-layout-flex). Also, the wp-container-xxx` classname will no longer be output for many blocks that use the default layout settings.

To-do

To 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):

image

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.

@andrewserong andrewserong marked this pull request as ready for review September 15, 2022 05:17
@andrewserong
Copy link
Author

@tellthemachines I believe this one is working correctly — it just depends on #3218 in order for the generated styles to be output.

Copy link
Contributor

@costdev costdev left a 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.

src/wp-includes/block-supports/layout.php Outdated Show resolved Hide resolved
src/wp-includes/block-supports/layout.php Outdated Show resolved Hide resolved
src/wp-includes/block-supports/layout.php Outdated Show resolved Hide resolved
tests/phpunit/tests/block-supports/layout.php Outdated Show resolved Hide resolved
tests/phpunit/tests/block-supports/layout.php Outdated Show resolved Hide resolved
tests/phpunit/tests/block-supports/layout.php Outdated Show resolved Hide resolved
*/
public function data_wp_get_layout_style() {
return array(
'should_return_empty_value_with_no_args' => array(
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

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!

src/wp-includes/block-supports/layout.php Outdated Show resolved Hide resolved
tests/phpunit/tests/block-supports/layout.php Outdated Show resolved Hide resolved
tests/phpunit/tests/block-supports/layout.php Outdated Show resolved Hide resolved
@andrewserong
Copy link
Author

Thanks for all the feedback @costdev! I believe I've implemented it all, so this should be ready for another look now 🙂

Copy link
Contributor

@tellthemachines tellthemachines left a 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">
Copy link
Contributor

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?

Copy link
Author

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!

Copy link
Contributor

@costdev costdev left a 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 👍

src/wp-includes/block-supports/layout.php Outdated Show resolved Hide resolved
Copy link
Contributor

@costdev costdev left a 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!

@andrewserong
Copy link
Author

Alrighty, now that #3273 has landed, it looks like this one should be good to commit now CC: @ockham

@andrewserong andrewserong force-pushed the try/backport-layout-block-support-refactor-part-2 branch from 18118b2 to d64d61d Compare September 20, 2022 01:00
@andrewserong
Copy link
Author

I've given this a rebase, and confirmed that it's still working correctly on top of trunk 👍

@audrasjb audrasjb assigned audrasjb and unassigned audrasjb Sep 20, 2022
@hellofromtonya hellofromtonya self-assigned this Sep 20, 2022
@hellofromtonya
Copy link
Contributor

I'm self-assigning for review and commit.

@costdev
Copy link
Contributor

costdev commented Sep 20, 2022

Test Report

Steps to Test

  1. Apply the PR.
  2. Activate the Twenty Twenty-Two theme. Ensure that there are no header customizations in the Site Editor.
  3. Navigate to the frontend.
  4. Inspect the header row.

Expected Results

  • ✅ The element should now have the .is-layout-flex class.
  • ✅ The styling should match that shown in the screenshot in the PR's description.

Environment

  • Server: Apache (Linux)
  • WordPress: 6.1-alpha-53344-src
  • Browser: Chrome 105.0.0.0
  • OS: Windows 10
  • Theme: Twenty Twenty-Two
  • Plugins: None activated.

Actual Results

  • ✅ The element has the .is-layout-flex class.
  • ✅ The styling matches that shown in the screenshot in the PR description.

Supplemental Artifacts

PR-3254

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a 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.

src/wp-includes/block-supports/layout.php Show resolved Hide resolved
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
@hellofromtonya
Copy link
Contributor

For this backport, I'll be reverting the DocBlock changes for wp_get_layout_style(). Why?

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 Optional. and adding the default Default is null.).

* Reverts changes not found in Gutenberg PR 40875
* Updates changes from Gutenberg PR 40875 for coding standards

WordPress/gutenberg#40875
Copy link
Contributor

@hellofromtonya hellofromtonya left a 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.

@hellofromtonya
Copy link
Contributor

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).

@hellofromtonya
Copy link
Contributor

DocBlock improvements suggestioned in this PR were committed via changeset https://core.trac.wordpress.org/changeset/54275.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
6 participants