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 legacy Group inner wrapper fix #3430

Conversation

tellthemachines
Copy link
Contributor

@tellthemachines tellthemachines commented Oct 11, 2022

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

This PR backports changes from WordPress/gutenberg#44660 to fix legacy Group block inner containers.


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.

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Let's make sure this is committed for RC 2 cc. @ockham @hellofromtonya.

@ockham
Copy link
Contributor

ockham commented Oct 13, 2022

Testing as follows:

  • On trunk, switch to a Classic Theme (e.g. TT1).
  • Create a new post, and insert the Group block.
  • Inside the Group block, insert the Columns block. Choose e.g. a 33/66 two-column layout.
  • In each column, insert a paragraph block, and enter some text.
  • Publish the post, and view on the frontend.
  • Check the page source for the Group block: There's no div with class wp-block-group__inner-container.

Then, switch to this branch, re-install npms and rebuild.

  • Reload the post. While there is no visual difference compared to trunk, the page source now has a div with class wp-block-group__inner-container.
trunk This branch
image image

A quick search of wp-block-group__inner-container in this repo reveals that it is indeed used for styling in Classic Themes, e.g. in Twenty Nineteen, Twenty Twenty, or Twenty Twenty One ✅


@tellthemachines Please don't forget to include testing instructions in PRs like this. I'm not very familiar with the Group block myself, and testing instructions in the originating Gutenberg PR and issue were rather sparse, and they didn't make it abundantly clear why this is an issue that warrants a fix during the RC phase.

It'd be great to have step-by-step instructions and before/after visuals that illustrate how badly this would break existing themes -- ideally in a more compelling way than my little frontend page source screenshots.

@dream-encode
Copy link
Contributor

I am happy to merge this since it's a very tightly scoped fix and appears to remedy a bug introduced during the 6.1 cycle. I agree with @ockham that I'd like to see some more testing to ensure this resolves the known issue, but, more importantly, doesn't have any unintended side effects.

@tellthemachines
Copy link
Contributor Author

@tellthemachines Please don't forget to include testing instructions in https://github.com/WordPress/gutenberg/pull/44660s like this. I'm not very familiar with the Group block myself, and testing instructions in the originating Gutenberg PR and WordPress/gutenberg#44636 were rather sparse, and they didn't make it abundantly clear why this is an issue that warrants a fix during the RC phase.

Ok, but this is not usual procedure for backports, and please remember this is only being committed during RC because it was left out of the pre-RC backports even though it was linked to the correct Trac ticket.

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

[Committer double signoff]

Approving. I've tried to cover both block themes and legacy themes, post editor and site editor, and frontend, and nothing stood out to me as broken.

@dream-encode
Copy link
Contributor

Merged into core in https://core.trac.wordpress.org/changeset/54633.

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.

4 participants