Remove data-align divs for themes that support layout #38613
Conversation
|
Size Change: -139 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
|
Thank you for this one, it's a big step forward for the consistency between markup. For testing purposes, it'd be good to collaborate on some test content we can vet together across themes. Took it for a quick spin, and here's the editor markup before: And after:
I tested some floated images in TT2, and note that floats outside a group have some discrepancy going in this theme, so that's a separate issue. Before:
After:
Ignoring the layout issue which is theme specific, the big difference here is the lack of left margin on a right floated image (and presumably right margin on a left floated image). I'd love a ton of eyes on this one, as it would so benefit the frontend/editor consistency. CC: @MaggieCabrera @kjellr |
|
Let's leave the "image" block out of our testing for now, I noticed that this block has some special behavior:
|
youknowriad
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
| @@ -117,6 +117,9 @@ export default { | |||
| ? ` | |||
| ${ appendSelectors( selector, '> *' ) } { | |||
| max-width: ${ contentSize ?? wideSize }; | |||
| } | |||
|
|
|||
| ${ appendSelectors( selector, '> :not(.alignleft):not(.alignright)' ) } { | |||
There was a problem hiding this comment.
I changed this that way the auto margins don't apply to left/right aligned blocks. Otherwise, the margins we had specifically for these blocks didn't apply at all. I think this has some small potential to impact FSE themes for aligned left/right blocks but this restores the original intent basically. I think it's fine but also curious about testing impact here.
There was a problem hiding this comment.
I guess also the "max-width" rule should be moved here maybe, let me know what you think?
There was a problem hiding this comment.
cc @jasmussen did you see this, any thoughts?
There was a problem hiding this comment.
To be sure I understand right, the desired behavior is that a floated image is omitted from max width and auto margins, so that it's up to a theme to decide whether it should be part of the main column, or not. Is that right?
If so, then that appears to work fairly well in this branch:
However, look at the first paragraph in that screenshot above, it no longer has a max-width for some reason, whereas it does in trunk:
Based on later comments in Maggie's testing content, it sounds like this is could be correct:
However it is inconsistent with the editor 🤔 :
There was a problem hiding this comment.
To be sure I understand right, the desired behavior is that a floated image is omitted from max width and auto margins, so that it's up to a theme to decide whether it should be part of the main column, or not. Is that right?
Yes I want to exclude left/right aligned blocks from the auto margins and the max width. But it's not up to the theme to decide whether it should be part of the main column. It can't be part of the main column at all. If you want something to be part of the main column, you need to wrap it in a group.
There was a problem hiding this comment.
Yep, sounds good to me. Did you see the Paragraph missing a max-width, issue, though? That's present in this branch but not in trunk.
There was a problem hiding this comment.
yes, it was actually a missing . it should be fixed in the last commit.
There was a problem hiding this comment.
I think there are some unwanted side effects to adding this additional specificity.
In the flow layout, align wide and align full are no longer respected because the max-width rule from the default alignment now takes higher precedence:
Using emptytheme:
| Before | After |
|---|---|
 |
 |
There was a problem hiding this comment.
Good catch.
I wonder if this would work? :where(:not(.alignleft):not(.alignright))
There was a problem hiding this comment.
@jasmussen's suggestion solves the alignment issue for me
|
Looking more here, I think the main problem to our refactoring here is going to be the "image block". Basically the image block adds a "div" to the frontend when it's left/right/center aligned. I tried solving this by adding the extra div to the editor as well, the problem is that while we could manage to get it work, I think it's not the right solution because the image block will still be a special case, and the alignments styles defined by the layout won't apply properly to that block since it will have an intermediary div before reaching the I think the best solution would be to make the image block work differently in themes with layout support, meaning, we should always add the The challenge is that there's a ton of image blocks (though this would be limited to image blocks with left/right alignments) out there and we need to find a way to only add that div for "classic themes" without layout support. The good news is that we have a precedent for a similar behavior: The group block. It has two divs for classic themes and one div only for new themes. I'm happy to try that and I think it's the main blocker for the current PR (it could also be its own PR) but would love some thoughts on the approach cc @mcsf @mtias @WordPress/gutenberg-core |
I'd like this, and would like to help if I can. |
|
This is a useful reference for different alignment cases: https://theamdemo.wordpress.com/2021/08/23/gutenberg-alignment/ |
I have a gist of that: https://gist.github.com/MaggieCabrera/799d09c819616354a1b8eb3605f4fc69 |
|
I've opened a separate PR to remove the wrapper div from the image block here #38613 |
youknowriad
force-pushed
the
remove/data-align
branch
from
fb92cf4 to
095a55c
Compare
|
I've rebased this PR, it now includes the changes to they image block which should issues like here #38613 (comment) So I think it's time to test again :) |
hypest
added
the
Mobile App - Automation
|
FWIW, added the |
|
Hey folks anyone able to give a ✅ here? |
|
Still seeing an issue: long captions won't break |
This matches the frontend though and my PR here only affects the editor. I'm happy to fix it as well but it will impact both editor and frontend. |
|
I think the caption styles was a regression of #38613 and not the current PR but I included a fix here. |
youknowriad
force-pushed
the
remove/data-align
branch
from
934f24b to
3428262
Compare
youknowriad
force-pushed
the
remove/data-align
branch
from
3428262 to
e12ac14
Compare
youknowriad
added
the
Needs Dev Note
|
Thank you Riad. This is an important one 🚀 |
Handle case of multiline plaintext Remove redundant escaping Handle case of multiline plaintext Remove redundant escaping Remove data-align divs for themes that support layout (WordPress#38613) Change Gallery block code ownership (WordPress#38722) * Remove mkevins from Gallery block codeowners * Add @geriux as codeowner for the gallery block Co-authored-by: Gerardo <gerardo.pacheco@automattic.com> Handle case of multiline plaintext Remove redundant escaping
cbravobernal
added
the
[Feature] Themes
|
THANK YOU for bringing this massive improvement! 🎉 |
alternative #38597
closes #37811
closes #33142
Historically, in the editor, we used to add wrapper divs with
data-alignattribute in order align things properly in the editor. This had several issues:With the recent updates and the introduction of the
layoutfeature, alignments are now styles provided by the framework and the layout feature, meaning we can remove these "data-align" divs and align the backend with the frontend markup.This PR tries to do so for themes that support the layout feature. It has some potential of breakage so we need to test this properly across all kind of themes and blocks.
Notes
Testing instructions
This PR is important but would take a lot of testing to check for potential regressions, I'd appreciate help here :)
Some of the styles for the blocks targeting
data-alignthings can be moved toclassic.cssmaybe after this PR.