Allow children of alignfull flow layouts to have root padding. #62670
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tellthemachines
added
[Type] Bug
tellthemachines
requested review from
richtabor,
ramonjd,
andrewserong and
aaronrobertshaw
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @fabiorubioglio. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
I've tested this with all the combinations of markup I could think of, but would be great to have a confidence check that this is viable as a fix before going ahead with the trac ticket and core PR rigmarole 😅 |
|
Size Change: +21 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This is testing pretty well for me @tellthemachines 👍
I really appreciate the test block markup, screenshots etc. Made it a lot easier to make sure I was testing the right things as I'm no layout expert 😅
✅ Could replicate the original issue
✅ Site editor correctly displayed padding and was consistent with frontend
✅ Post editor applied padding and was consistent with frontend
✅ Padding applied consistently across TT4, TT3, Assembler, and Powder themes
✅ Didn't spot any differences in themes without useRootPaddingAwareAlignments
✅ Multiple levels of nested group blocks with and without content width work as expected
I think this is ready for a couple of unit test updates and the backport. It looks like we only have coverage of what generated for these styles in the PHP theme.json tests. The useGlobalStylesOutput hook tests passed.
|
Thanks for the review @aaronrobertshaw ! Tests are updated and sync PR is up: WordPress/wordpress-develop#6854 |
🚀 Appreciate the quick turnaround, the PHP unit tests are now passing If you're not a fan of updating the use global styles hook's tests because it's just testing style generation, perhaps a better option would be to add an e2e like those added in the specificity changes PR, to guard against regressions here? |
These styles are pretty unstable - they've been updated many times since initial implementation - so I'm reluctant to add more tests for them as it means more maintenance overhead for little benefit. A large part of the problem is there has never been agreement on what precisely |
andrewserong
added
the
Backport to WP 6.7 Beta/RC
andrewserong
approved these changes
Jun 20, 2024
There was a problem hiding this comment.
Nice work, this is testing well for me, too! It's a tiny bit unfortunate that we need to add more complexity to the selectors, but it's a good issue to solve 👍
Maybe if at some point we go for one or two release cycles without making a change to these styles, we can consider them stable enough to invest in tests 😅 but for now I think it's more trouble than it's worth.
In principle, I agree with the idea of trying to ensure we've got good test coverage of the styling rules we're outputting. In practice for this feature I agree it's likely more trouble than it's worth as we'd just be string matching for now.
Arguably, there could be a case to be made to ensure that what's output from use-global-styles-output.js matches the PHP version to ensure no-one makes any typos, but at this stage, I think it'd be good to treat how we reliably test the root padding styles as a separate exploration from this fix. The important thing IMO is that multiple folks have taken the PR for a manual spin to test with block markup in a variety of situations and themes.
LGTM! 🚀
I've added the Backport to WP Beta/RC label since there's a JS change in here. Please update the labels if this isn't right, though 🙂
Ideally there wouldn't even be this duplication in the code! But yes it's something to look at later 😅 |
ellatrix
pushed a commit
that referenced
this pull request
Jun 25, 2024
Unlinked contributors: fabiorubioglio. Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: alexandrebuffet <alexandrebuffet@git.wordpress.org>
|
I just cherry-picked this PR to the wp/6.6-rc-1 branch to get it included in the next release: 510684e |
ellatrix
added
Backported to WP Core
ellatrix
pushed a commit
that referenced
this pull request
Jun 25, 2024
Unlinked contributors: fabiorubioglio. Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: alexandrebuffet <alexandrebuffet@git.wordpress.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #62597.
When a container with inner content width is nested inside another container that is both full width and flow type layout (without content width), the inner container should still receive root padding.
Testing Instructions
Test markup
useRootPaddingAwareAlignmentsenabled in its theme.json settings.Testing Instructions for Keyboard
Screenshots or screencast
Before:
After: