Unify how editor alignments are applied across blocks #21822
Conversation
youknowriad
added
[Feature] Blocks
| children, | ||
| tagName = 'div', | ||
| __unstableIsHtml, | ||
| wrapperProps: parentWrapperProps, |
There was a problem hiding this comment.
This is just needed for the image block now, but we can remove it if ever we refactor it to use the "align" hook instead of a custom implementation.
There was a problem hiding this comment.
Can we not use the block context for this?
There was a problem hiding this comment.
I'm not sure how you'd do so, retrieve the context value, alter it and add a new provider? It would work but it's also not great code either, I'd rather remove this at some point (after the image refactor).
There was a problem hiding this comment.
Actually, we can probably use getEditPropsWrapper
There was a problem hiding this comment.
I meant adding it to useContext( BlockContext ) below
packages/block-editor/src/components/block-list/block-wrapper.js
Outdated
Show resolved
Hide resolved
youknowriad
force-pushed
the
add/unify-editor-alignments
branch
from
9f3700e to
d945522
Compare
|
Size Change: -530 B (0%) Total Size: 844 kB
ℹ️ View Unchanged
|
| @@ -352,6 +352,9 @@ export class ImageEdit extends Component { | |||
| sizeSlug, | |||
| } = attributes; | |||
|
|
|||
| const wrapperProps = { | |||
| 'data-align': align, | |||
| [data-align="right"] > .wp-block-cover, | ||
| [data-align="left"] > .wp-block-cover-image, | ||
| [data-align="right"] > .wp-block-cover-image { | ||
| max-width: $content-width / 2; |
There was a problem hiding this comment.
Why is width being defined here?
There was a problem hiding this comment.
I'm just curious, it's not a comment to this PR.
There was a problem hiding this comment.
Just guessing
I believe if we don't do it and align a block (left or right) we won't notice any change as otherwise it will stay with a 100% width
youknowriad
requested review from
ajitbohra,
jorgefilipecosta,
mkaz,
Soean and
talldan
as code owners
|
Sorry for being so late to this review. This feels like an excellent first step onto bigger things, and the reduced and simplified markup is so lovely. Also bonus points for including the float margin. Great work all around. There's one thing I noticed, technically it's not a regression from master — master is actually broken here 😱 — but worth noting so as to not grow worse. I notice because I recently merged #21608 and wanted to be sure things didn't regress. It looks as though a centered image isn't centered in either master or this branch, in the editor only. But in master, at least the Master:
You can see there's a text-align: center there which doesn't work (that's the bug), but at least it's there. This branch:
It seems the quick fix would be to just remember to include some centering stuff? As mentioned, it works on the frontend, so what we could output in the editor could be something like: This appears to work for me in the editor, and should not be needed on the frontend, but I can't think of how it might cause issues. |
I think that's a good fix, I noticed this too but left it separate but I'm ok including it here. The reason there's no classnames here is to stay consistent, we don't use these classnames in the editor in general (not just within this PR), we tried using some of them on the image block recently but the inconsistency it created with other blocks that rely on data-align was not great. Ideally, we'd remove data-align and only use the classnames to match frontend but since this would require updates and changes to frontend markup for the float alignments essentially, it's left separate. I'll push the code, I'll just replace |
|
Ship it then! It seems like a good path forward. |
|
Yes, in one of the next iteration we'll try out the classnames again, but for all blocks to use it consistently rather than just the image block. This PR is a key step towards that. |
|
All good, go forth and conquer! |
|
Guys, I might be absolutely wrong but I think I've found some sort of a regression here. I noticed #21885 reporting that a centre-aligned image wasn't appearing as centre-aligned when they were using the Gutenberg plugin. I started testing it out and eventually landed on this commit and PR.
@jasmussen I'm not sure if this issue is specific to the theme, but this workaround only seems to work when the Also, after you add an image, it stays aligned to the very left. I'm not sure if this is related to this PR too though. Let me share a screencast: https://take.ms/S54Si which may help understand the issue better. Do you think the changes in this PR might've introduced these problems? If that is the case, could you point me towards a potential fix and I can quickly work on a PR? If that is not the case, please excuse my concern, I might've just not done the investigation well. I can create a new issue in that case. Thanks! ❤️ |
|
I’d like to add, adding |
|
Excellent catch, @nfmohit-wpmudev. I can confirm that:
Centered but not resized:
Centered and resized:
At a glance, this change fixes it. Instead of: if we do this: it appears to work for both resized and unresized images. @youknowriad any concerns with that one? Let me know if you'd like me to make a PR. |
|
Hey @jasmussen ! Thank you very much for the confirmation. I'm very sorry, I didn't notice this response and submitted #21911, is that okay? Thanks! |
|
Of course that's okay, it's incredibly helpful. Thank you! |
|
Thank you very much! Also, do you possibly have any idea why the image block gets added originally to the very left (like, left-aligned)? Here's a screencast. I can confirm that this doesn't happen on the core without the plugin. It aligned with the content when first added. Thank you! |
|
Yep, I believe that's an issue with the TwentyTwenty theme happening after some of these alignment refactors. It's unfortunate, but I believe, necessary, so as to improve the editor styles overall. |
|
Thank you for the confirmation. That's completely understandable. Just wanted to confirm that it is something that needs to be updated on the theme's end. |
While working on the
lightBlockWrapperAPI, we experimented on the Image block about how we can keep the frontend similar to the backend for alignments. It worked for this particular block but it's not something we can generalize yet to all blocks (especially the ones relying on the align hook).This PR partially reverts the image block alignments change and at the same time updates the "align" hook to support the new "lightBlockWrapper" behavior. This means lightBlockWrapper doesn't mean we're consistent with frontend for aligned blocks but it also means that all blocks in the editor behaves consistently when it comes to alignment (fixes #21685)
So this is an intermediate step before the bigger alignments rethinking that is happening separately.