Alignment styles: Centre blocks using grid not margins #32231
Merged
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
7 tasks
|
Size Change: -568 B (0%) Total Size: 1.86 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
May 26, 2021
lib/block-supports/layout.php
Outdated
| $style .= '}'; | ||
|
|
||
| $style .= ".wp-container-$id > .alignwide { max-width: " . esc_html( $wide_max_width_value ) . ';}'; | ||
| $style .= ".wp-container-$id > .alignwide { width: " . esc_html( $wide_max_width_value ) . ';}'; |
There was a problem hiding this comment.
This doesn't seem to be responsive, it doesn't adapt to the screen size if it's bigger than the width. (thus the previous usage of max-width)
There was a problem hiding this comment.
We went the grid way and it seems to be working fine responsively now
|
What I like about this solution though is that it seems to make left/right alignment work well even inside containers with a "content size". Would be cool if we find a way to solve the responsiveness. |
scruffian
changed the title
|
Yeah, as Maggie said, this should be responsive since we changed the approach to use grid. |
aristath
reviewed
May 27, 2021
aristath
approved these changes
May 27, 2021
Co-authored-by: Ari Stathopoulos <aristath@gmail.com>
|
I'm testing with tt1-blocks, I'm not seeing a difference between wide and full alignments after this PR (both show as "wide" I think) |
|
Something else I noticed is that while we were considering enabling "floats" (align left and right) for containers with a layout defined, I didn't manage to make floats work with this approach (grid container), maybe you all know how we could do that? |
there's the option of using |
|
This is a pretty wild one. At a first glance and on a basic level, this worked for me:
The grid also didn't seem to cascade into other grid containers, so that's good:
The part where it's wild is that by making the container into a grid container, every block inside is placed on grid-auto-rows:
... which also means you could theoretically add a
— not that anyone should, because the margins of blocks are still present:
Unfortunately that's also where things started to fall apart for me. The grid prevents margin collapsing:
This can be seen as an opportunity — with all that the grid container offers, we might want to leverage some of the harmonious spacing features that are possible when using Riad mentioned floats — those are possible inside groups:
Margins also collapse just fine inside groups, since unless they have All that is to say — I think there's huge potential here, and that this is likely something we'll want to do. In #16998 similar thoughts are being discussed, around how we can accomplish incredible layouts using something like this, by defining wide and full-wide as grid-column presets that can easily be adjusted, by having ad-hoc resize handles appear in the gutters between blocks. But I do worry about what the lack of margin collapsing does for existing themes, even if it only affects block themes; it's quite a change since most post-content containers have layout, and suddenly paragraphs have double-margins. |
|
Should we revert this one until we resolve the issue(s) outlined above? |
|
Yes, I think we should revert this one. The "grid" container should be a possibility but I think it makes more sense as its own layout type and not the default one. |
|
PR #32312 reverts this 👍 |
7 tasks
aristath
added a commit
that referenced
this pull request
May 28, 2021
|
I think we should reconsider this. Could we add a theme_supports for "inherit layout", so that theme developers are able to opt in to the feature and adjust their themes accordingly? Alternatively we could look at removing margins of blocks inside "inherit layout" containers. |
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.
Description
To centre the content inside blocks that use the "inherit layout" we are using "margin: auto" on every block. This will conflict with the new margin supports being added to blocks. To overcome this we added !important declarations to the alignment styles for blocks that use the "inherit layout" setting (in #30608). However this mean that left and right alignment settings in blocks won't work.
This PR takes a different approach to centering content inside blocks that use the "inherit layout" setting - instead of margins we use CSS grids. The container is given "display: gird". This means we can now set margins on these blocks without messing up the page alignments. Props to @MaggieCabrera for help with grid.
This requires declaring an explicit width to these blocks, rather than relying on the natural flow of the document.
Adding margins to these blocks will make them wider than the page content, but I think that is the expected behaviour. I have also added "box-sizing: border-box" to these blocks, so that the padding settings don't make the blocks themselves wider.
How has this been tested?
Switch to empty theme
Try adding normal content and ensure that its centered on the page
Try adding full width and wide width blocks and ensure they are centred and the correct width
Try adding left/right aligned blocks
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).