VisualEditor: Remove wide and full alignment CSS and alignment classes on the root container #66352
Conversation
andrewserong
added
[Type] Bug
|
Size Change: -149 B (-0.01%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
| @@ -272,7 +272,7 @@ function VisualEditor( { | |||
| 'is-layout-flow': ! themeSupportsLayout, | |||
| }, | |||
| themeSupportsLayout && postContentLayoutClasses, | |||
| align && `align${ align }` | |||
| align && ! [ 'wide', 'full' ].includes( align ) && `align${ align }` | |||
There was a problem hiding this comment.
Any reason to keep left/right/center alignments?
There was a problem hiding this comment.
Mostly out of caution as post content blocks that use left/right alignment look quite different to a center aligned column and the reported issue was to do with wide/full alignments. Happy to remove this altogether if we don't want the left/right alignment.
I believe @jasmussen and @richtabor have more thoughts about the behaviour here, so I'll hold off on any updates until they get a chance to look at this.
From my perspective I think there are good arguments both ways between attempting to display some alignments in the abstracted (template preview off) mode, and skipping them entirely. I don't mind which way we go with it, so I'll leave this PR open for now until there's a chance for more feedback on the approach.
TL;DR — let me know if anyone would like to remove the left/right/center alignments here and I'll remove them 🙂
There was a problem hiding this comment.
I think it would make sense to leave left/right alignments because they don't cause problems 😅 and so we can have some degree of WYSIWYG.
There was a problem hiding this comment.
Personally, I'm for consistency here. Left/right alignments for instance cause the whole thing to be "float" displayed, which IMO doesn't matter in this context at all and may actually create bugs.
So instead of a half baked WYSIWYG, I think we should rather embrace that this is an abstracted view of the post editor and not meant to be full WYSIWYG in terms of layout.
There was a problem hiding this comment.
Oh hang on, actually I don't think there would be right/left align classes here; the Content block should only have wide/full enabled.
When we set a constrained layout to justify left/right it doesn't use alignright or alignleft classes (or floats), it uses margin-left: auto or margin-right: auto. So it should be fine to remove the alignments here altogether! It won't make any difference to content justification.
There was a problem hiding this comment.
Sounds good! I'll remove the align classes here 👍
There was a problem hiding this comment.
Done in 6d8f1cf
|
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 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. |
andrewserong
force-pushed
the
try/remove-full-and-wide-alignments-from-abstracted-post-editor
branch
from
7e36a8d to
dcfc7f2
Compare
| @@ -272,7 +272,7 @@ function VisualEditor( { | |||
| 'is-layout-flow': ! themeSupportsLayout, | |||
| }, | |||
| themeSupportsLayout && postContentLayoutClasses, | |||
| align && `align${ align }` | |||
| align && ! [ 'wide', 'full' ].includes( align ) && `align${ align }` | |||
There was a problem hiding this comment.
I think it would make sense to leave left/right alignments because they don't cause problems 😅 and so we can have some degree of WYSIWYG.
|
Thanks for the review @tellthemachines! 🙇 I'll hold off on merging just for now and see if there's any further feedback from @jasmussen, @richtabor or @WordPress/gutenberg-design. If not, we can try merging this in tomorrow or early next week. |
andrewserong
changed the title
|
Thanks for exploring! Rich and I were able to connect on this, to find out more fundamentally what was at play here. It's complex, though already mostly captured in the discussion above. It's just to say, if Rich has thoughts here, that's important to veer towards. High level, there's an issue at play, where we have a abstracted post/page editor, which in many themes is mostly WYSIWYG. It is, certainly, and should be, affected by color, typography, and every other property we can supply it with, to give you a decent idea of what the end result is. But the abstracted editor is also just the title and the Content block. Any ancestor containers present in the Single templates, a Group with variable paddings, nested Columns blocks, sidebars left or right or both, are stripped from this view. This makes it inevitable that in some themes, this abstracted view differs quite a lot from the theme design itself. Simply put, not all themes have a centered column and whitespace left and right. As much as is reasonable and possible, it's best to veer towards as much WYSIWYG in the abstracted editor as we can. And still balance that against a good writing experience. What Rich and I found in investigating this, is that trunk Gutenberg, except for one edge case, actually does this, treading that careful line very well. The edgecase we found was if you assign a wide or full-wide alignment to the Content block when it's inside a Group that defines content widths, and then move the Content block outside of that. In this case, the wide or fullwide attribute still exists, but the button to toggle the attribute off is hidden, and yet the attribute is treated as if the element should be full-wide. This edgecase, I guess, is technically a bug. It seems like this PR fixes that, if that's the case, great. But otherwise, I was actually going to close #52163 and open a new one just for this edgecase. Let me know! |
|
Thanks for laying that out @jasmussen!
Yes, this PR fixes that edge case. It also means that any Content block set in a template that has Full or Wide alignment attached to it will not have that class name attached to it in the abstracted editor. The more that I think about it, the more I believe this PR is probably the right way to go, as I think that effort to display So I'm still in favour of merging this PR. For templates/themes that set the Content block at the very root of the document and use flow/default layout, the editor will still display edge-to-edge for that use case of Page templates that are intended to be full wide to the very edge of the screen. Also with the idea you've raised in #66429 to better expose the toggle to preview the template, I think we're in a good place to proceed with this PR. Let me know if anyone has any objections to landing this, and if not, I'll intend to press "Squash and merge" on Monday 🙂 |
|
I've no objection to merging. Though if Rich gets a chance (unclear he will before Monday), I'd appreciate, just so we don't regress some nuance in some of the behavior, where for example an image that was actually intentionally set to full-wide doesn't stop working inside this abstracted Content block, or the likes. I'll drop a link in case he gets a chance to look. |
|
Thanks for the update, Joen! I'll leave this PR open until @richtabor has a chance to give it a look. Since this fixes something that's a bit of an edge case, I'm happy to leave this open for a while as it doesn't seem urgent. |
andrewserong
force-pushed
the
try/remove-full-and-wide-alignments-from-abstracted-post-editor
branch
from
6d8f1cf to
fcfa837
Compare
What?
Fixes #52163. Note, this does not fix #60311 which had the opposite feedback. If this PR lands, we'll effectively be saying that #60311 is a "won't fix" 🙂
This PR proposes removing the wide and full alignment CSS on the root container of the visual editor. I'm happy to close out this PR if it isn't a desired path forward.
Why?
This CSS was in place so that the post and page editors could more closely resemble how a post might look in the context of a template. However, ever since the editors were unified, it's possible for users to switch on template preview mode (or in the case of page editing in the site editor, switch it off). In #52163 the feedback / discussion is that displaying post content full width in the post and page editors isn't a WYSIWYG experience and can make the editor feel broken.
The suggestion there was to remove the behaviour that detects wide / full width alignment and attempts to display content to the wide / full sizes in the post editor. Note: for posts and pages that use templates that do use content width, wide and full alignments should still be available within the post content, so for example setting a Cover or Group block to wide or full in regular templates should work as before.
How?
alignCSSrules that attempted to make the editor (with template preview switched off) look more WYSIWYGTesting Instructions
Single item: PostBefore
Note: on
trunk, in this case, the content would have displayed edge-to-edge in the post editor: