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 foundation for Layout block support refactor (part 1) #3205

Conversation

andrewserong
Copy link

@andrewserong andrewserong commented Sep 7, 2022

This PR is the beginnings of an attempt to backport the Layout block support refactor from Gutenberg to WordPress core for the 6.1 release. The goal is to attempt to do this in parts.

  • Part 1 (this PR) is to backport the foundation for outputting centralised styles
  • Part 2 will be to then backport layout.php in a follow-up, once the prerequisite issues have been resolved

Note that in the progress of working on this PR, it also pulls in parts of unrelated changes.

Scope

  • Include centralised Layout styles output
  • Include root padding / root layout styles
  • Include feature level selectors as they are present in some of the merged files

Still to-do

Included PRs

This change backports the following changes (while also being slightly different in order to align with core files):

Note that it doesn't entirely port over #40875 — the remaining PHP changes for that PR will be explored in a separate PR targeting layout.php

What does this PR do, primarily?

The primary change that this PR seeks to merge in, is the output of root layout styles. On the site's frontend, you should see rules in the global styles output using classnames such as is-layout-flex and is-layout-constrained. Note that outputting these classnames into block markup is not included in this PR, as that forms part of the changes in layout.php which will be addressed separately.

Trac ticket: https://core.trac.wordpress.org/ticket/56467

Gutenberg tracking issue: WordPress/gutenberg#43440


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
Copy link
Author

Update: today I've been mostly focused on reviewing PRs in the Gutenberg repo that also touch the files included in this PR. I suspect there'll be a fair few more changes landing in Gutenberg shortly, but so far I think the rough scope of this PR seems to be an okay first step toward backporting the Layout refactor.

I'll follow-up early next week to give this a fresh overhaul to roll in recent changes, and see about getting this ready for review.

@andrewserong andrewserong force-pushed the try/backport-layout-block-support-refactor branch from 96b0844 to 9761152 Compare September 12, 2022 05:25
@andrewserong
Copy link
Author

Update: I think most of the changes are now migrated over correctly for these files, and unit tests are finally passing 🎉

One outstanding task is that I haven't yet migrated over the test for the feature level selectors that was included in WordPress/gutenberg#42087 by @aaronrobertshaw as I wasn't too sure where or how we should best register blocks in core tests. I'll look into that further tomorrow.

CC: @tellthemachines I believe the root padding and layout changes have all been migrated over — except for the changes to layout.php. The intention in this PR is to try to merge the foundations of the layout support in one step, and then in a subsequent PR merge the layout.php changes (once the style engine has been merged), so that it's a bit easier to progress.

Since tests are now passing, I'll hook up the trac ticket and switch this over to ready for view. It's quite possible that I've missed things in the process, so please do let me know if anyone catches things that should be fixed up. Thanks!

@andrewserong andrewserong changed the title [WIP] Backport foundation for Layout block support refactor Backport foundation for Layout block support refactor (part 1) Sep 12, 2022
@andrewserong andrewserong marked this pull request as ready for review September 12, 2022 08:01
@aaronrobertshaw
Copy link

I wasn't too sure where or how we should best register blocks in core tests. I'll look into that further tomorrow.

@andrewserong, we might be able to avoid registering a new block within the bootstrap.php for the feature-level selectors test. From memory, we needed to do that because WordPress was loaded before the tests ran, meaning theme.json generated block metadata too soon effectively ignoring any block registration we did in the specific tests. Now the image block adopts the feature level selectors, it's probably time to switch the test to use that core block.

I'm out of time today but will touch base with you tomorrow on this one.

@andrewserong
Copy link
Author

Thanks for following up, Aaron! In that case, we might be better off adding the test for the feature level selectors once the image block's block.json file is updated, which should happen as part of #3154. I might leave the scope of this PR as-is, then, but happy to continue investigating if anyone thinks we should pursue it in this PR.

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!

src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
@andrewserong
Copy link
Author

@ockham, I believe this one should be ready for final review/commit now that we've got approval from @tellthemachines (thanks!)

To recap:

  • This PR does not require the style engine changes to be merged in first, as it doesn't change layout.php, so should be able to be safely committed at any time.
  • Once the style engine PR is merged (Style engine: migrate functions, classes and tests #3199), I can put together a separate PR that backports layout.php.
  • Once this PR and the style engine PR is merged, we can put together a separate PR to backport the spacing presets work (CC: @glendaviesnz).

@audrasjb audrasjb self-assigned this Sep 14, 2022
@hellofromtonya
Copy link
Contributor

hellofromtonya commented Sep 14, 2022

This PR was committed in https://core.trac.wordpress.org/changeset/54159. However, the added tests from this PR failed.

During investigation, I found that this PR tests pass when the Style Engine's commit is not applied, but fails when it is.

So this PR's commit was reverted.

Next step:

  • Rebase this PR against the current trunk (which includes the Style Engines commit.
  • Then investigate to find the root cause.
  • Fix the root cause.

Then it'll be ready again for commit consideration.

@ockham
Copy link
Contributor

ockham commented Sep 14, 2022

This PR was committed in https://core.trac.wordpress.org/changeset/54159. However, the added tests from this PR failed.

FWIW, here's a link to the failing tests.

@ockham
Copy link
Contributor

ockham commented Sep 14, 2022

Sharing some more findings (also posted in Slack):

Weirdest thing, I tried rebasing locally but can’t reproduce the error here 🤔 (At least not when only running npm run test:php — --group=themes — running the full test suite now.)

Furthermore, it looks like #3244 has the same test failures — and that PR has a totally unrelated change (introduces a new function as an alias for an existing class method).

Heisenbug? 😕

@hellofromtonya
Copy link
Contributor

Follow-up:

When applying this PR on top of the current trunk, the tests pass locally and within the CI. I opened a separate PR #3249 as a confidence check and to isolate the code changes. Tests pass ✅

During investigation, I found that this PR tests pass when the Style Engine's commit is not applied, but fails when it is.

Repeating this same process, tests pass locally in both cases ✅

This leads me to believe the issue from earlier may possibly be related to something within the wp-env remote deps, such as Docker, Composer, GitHub, etc. Whatever that something was, it appears to be resolved.

So, I'm recommitting this PR, but just to make sure, recommitting it on top of trunk.

@hellofromtonya
Copy link
Contributor

Committed via https://core.trac.wordpress.org/changeset/54162.

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
Development

Successfully merging this pull request may close these issues.

6 participants