Add layout styles from Post Content block to post editor #44258
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -28,13 +28,16 @@ import { | |||||
| __unstableUseMouseMoveTypingReset as useMouseMoveTypingReset, | ||||||
| __unstableIframe as Iframe, | ||||||
| __experimentalRecursionProvider as RecursionProvider, | ||||||
| __experimentaluseLayoutClasses as useLayoutClasses, | ||||||
| __experimentaluseLayoutStyles as useLayoutStyles, | ||||||
| } from '@wordpress/block-editor'; | ||||||
| import { useEffect, useRef, useMemo } from '@wordpress/element'; | ||||||
| import { Button, __unstableMotion as motion } from '@wordpress/components'; | ||||||
| import { useSelect, useDispatch } from '@wordpress/data'; | ||||||
| import { useMergeRefs } from '@wordpress/compose'; | ||||||
| import { arrowLeft } from '@wordpress/icons'; | ||||||
| import { __ } from '@wordpress/i18n'; | ||||||
| import { parse } from '@wordpress/blocks'; | ||||||
|
|
||||||
| /** | ||||||
| * Internal dependencies | ||||||
|
|
@@ -82,20 +85,48 @@ function MaybeIframe( { | |||||
| ); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Given an array of nested blocks, find the first Post Content | ||||||
| * block inside it, recursing through any nesting levels. | ||||||
| * | ||||||
| * @param {Array} blocks A list of blocks. | ||||||
| * | ||||||
| * @return {Object} The Post Content block. | ||||||
| */ | ||||||
| function findPostContent( blocks ) { | ||||||
|
There was a problem hiding this comment. Could we help our future selves here and add a JS doc comment? |
||||||
| for ( let i = 0; i < blocks.length; i++ ) { | ||||||
| if ( blocks[ i ].name === 'core/post-content' ) { | ||||||
| return blocks[ i ]; | ||||||
| } | ||||||
| if ( blocks[ i ].innerBlocks.length ) { | ||||||
| const nestedPostContent = findPostContent( | ||||||
| blocks[ i ].innerBlocks | ||||||
| ); | ||||||
|
|
||||||
| if ( nestedPostContent ) { | ||||||
| return nestedPostContent; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| export default function VisualEditor( { styles } ) { | ||||||
| const { | ||||||
| deviceType, | ||||||
| isWelcomeGuideVisible, | ||||||
| isTemplateMode, | ||||||
| editedPostTemplate = {}, | ||||||
| wrapperBlockName, | ||||||
| wrapperUniqueId, | ||||||
| } = useSelect( ( select ) => { | ||||||
| const { | ||||||
| isFeatureActive, | ||||||
| isEditingTemplate, | ||||||
| __experimentalGetPreviewDeviceType, | ||||||
| getEditedPostTemplate, | ||||||
| } = select( editPostStore ); | ||||||
| const { getCurrentPostId, getCurrentPostType, getEditorSettings } = | ||||||
| select( editorStore ); | ||||||
| const _isTemplateMode = isEditingTemplate(); | ||||||
| let _wrapperBlockName; | ||||||
|
|
||||||
|
|
@@ -105,10 +136,17 @@ export default function VisualEditor( { styles } ) { | |||||
| _wrapperBlockName = 'core/post-content'; | ||||||
| } | ||||||
|
|
||||||
| const supportsTemplateMode = getEditorSettings().supportsTemplateMode; | ||||||
|
|
||||||
| return { | ||||||
| deviceType: __experimentalGetPreviewDeviceType(), | ||||||
| isWelcomeGuideVisible: isFeatureActive( 'welcomeGuide' ), | ||||||
| isTemplateMode: _isTemplateMode, | ||||||
| // Post template fetch returns a 404 on classic themes, which | ||||||
| // messes with e2e tests, so we check it's a block theme first. | ||||||
| editedPostTemplate: supportsTemplateMode | ||||||
| ? getEditedPostTemplate() | ||||||
| : {}, | ||||||
| wrapperBlockName: _wrapperBlockName, | ||||||
| wrapperUniqueId: getCurrentPostId(), | ||||||
| }; | ||||||
|
|
@@ -122,16 +160,13 @@ export default function VisualEditor( { styles } ) { | |||||
| themeHasDisabledLayoutStyles, | ||||||
| themeSupportsLayout, | ||||||
| assets, | ||||||
| isFocusMode, | ||||||
| } = useSelect( ( select ) => { | ||||||
| const _settings = select( blockEditorStore ).getSettings(); | ||||||
| return { | ||||||
| themeHasDisabledLayoutStyles: _settings.disableLayoutStyles, | ||||||
| themeSupportsLayout: _settings.supportsLayout, | ||||||
| assets: _settings.__unstableResolvedAssets, | ||||||
| isFocusMode: _settings.focusMode, | ||||||
| }; | ||||||
| }, [] ); | ||||||
|
|
@@ -154,7 +189,7 @@ export default function VisualEditor( { styles } ) { | |||||
| borderBottom: 0, | ||||||
| }; | ||||||
| const resizedCanvasStyles = useResizeCanvas( deviceType, isTemplateMode ); | ||||||
| const globalLayoutSettings = useSetting( 'layout' ); | ||||||
| const previewMode = 'is-' + deviceType.toLowerCase() + '-preview'; | ||||||
|
|
||||||
| let animatedStyles = isTemplateMode | ||||||
|
|
@@ -183,25 +218,68 @@ export default function VisualEditor( { styles } ) { | |||||
|
|
||||||
| const blockSelectionClearerRef = useBlockSelectionClearer(); | ||||||
|
|
||||||
| // fallbackLayout is used if there is no Post Content, | ||||||
| // and for Post Title. | ||||||
| const fallbackLayout = useMemo( () => { | ||||||
| if ( isTemplateMode ) { | ||||||
| return { type: 'default' }; | ||||||
| } | ||||||
|
|
||||||
| if ( themeSupportsLayout ) { | ||||||
| // We need to ensure support for wide and full alignments, | ||||||
| // so we add the constrained type. | ||||||
| return { ...globalLayoutSettings, type: 'constrained' }; | ||||||
| } | ||||||
| // Set default layout for classic themes so all alignments are supported. | ||||||
| return { type: 'default' }; | ||||||
| }, [ isTemplateMode, themeSupportsLayout, globalLayoutSettings ] ); | ||||||
|
|
||||||
| const postContentBlock = useMemo( () => { | ||||||
| // When in template editing mode, we can access the blocks directly. | ||||||
| if ( editedPostTemplate?.blocks ) { | ||||||
| return findPostContent( editedPostTemplate?.blocks ); | ||||||
|
There was a problem hiding this comment. I can reproduce error that @andrewserong noticed but only sometimes. Most of the times it works for me. When it does fail I suspect it's because postContentBlock is undefined (and therefore trying to access the attributes below fails). Replacing with
But it exposes another error triggered by the component 🤣
Not sure why yet. I can't reproduce either error in trunk. There was a problem hiding this comment. Hmm, I've only managed to repro the error once on TT2, and not at all on Empty theme. But I think something's messed up because previous customisations are now appearing as the default template state 😅 There was a problem hiding this comment. Ok I fixed the failure if Post Content block is removed from the template by accessing layout from attributes via optional chaining instead of destructuring. The reason returning empty object from |
||||||
| } | ||||||
| // If there are no blocks, we have to parse the content string. | ||||||
| // Best double-check it's a string otherwise the parse function gets unhappy. | ||||||
| const parseableContent = | ||||||
| typeof editedPostTemplate?.content === 'string' | ||||||
| ? editedPostTemplate?.content | ||||||
| : ''; | ||||||
|
|
||||||
| return findPostContent( parse( parseableContent ) ) || {}; | ||||||
| }, [ editedPostTemplate?.content, editedPostTemplate?.blocks ] ); | ||||||
|
|
||||||
| const postContentLayoutClasses = useLayoutClasses( postContentBlock ); | ||||||
|
|
||||||
| const blockListLayoutClass = classnames( | ||||||
| { | ||||||
| 'is-layout-flow': ! themeSupportsLayout, | ||||||
| }, | ||||||
| themeSupportsLayout && postContentLayoutClasses | ||||||
| ); | ||||||
|
|
||||||
| const postContentLayoutStyles = useLayoutStyles( | ||||||
| postContentBlock, | ||||||
| '.block-editor-block-list__layout.is-root-container' | ||||||
| ); | ||||||
|
Comment on lines
+261
to
+264
There was a problem hiding this comment. I think there appears to be a difference in the rules that are being output here versus trunk 🤔 I noticed that there's an issue with the middle content justification with the Cover block set to wide alignment in TwentyTwentyTwo:
This appears to be the rule that isn't present / winning out:
There was a problem hiding this comment. I wonder if the issue is that There was a problem hiding this comment. Those styles that are overriding the middle justification are the TT2 root padding logic that I thought were going to be removed after the Core root padding styles became available. I'm reluctant to either increase specificity of the default layout styles or add This should probably be fixed in TT2 instead cc @carolinan @scruffian There was a problem hiding this comment.
Agreed, it'd be better not to have to add container rules, or explicit rules for the default justification state 🤔
It's a tricky one... because TT2 has been used as a base for many other themes, it might not just TT2 out in the wild that has this issue. (E.g. the rule appears to be part of BlockBase https://github.com/Automattic/themes/blob/23b9090090279c95788357bb634fd2e57c3973db/blockbase/sass/base/_alignment.scss#L58 which many themes have been built on top of, too). Just trying to think of other options... would it work to update the change to this line in this PR to include.block-editor-block-list__layout.is-root-container as well (as on trunk)? Or did we we need to remove it for the postContentLayoutStyles to work? Since they already have the selector attached, I was wondering if they can coexist. From quickly adding it back in in my local environment, it appears to get all three content justifications working in TT2 for me, but it's highly likely I'm missing some context 😅
There was a problem hiding this comment.
I did remove it initially so that custom content widths would work, but because we since increased the specificity of the default rules it seems to be working ok. I just tried pushing that change now, hopefully it doesn't break anything else 😅
I'm not sure how much of a concern that should be to us, because the temporary status of these rules is explicitly called out in the TT2 stylesheet. |
||||||
|
|
||||||
| const layout = postContentBlock?.attributes?.layout || {}; | ||||||
|
|
||||||
| // Update type for blocks using legacy layouts. | ||||||
| const postContentLayout = | ||||||
| layout && | ||||||
| ( layout?.type === 'constrained' || | ||||||
| layout?.inherit || | ||||||
| layout?.contentSize || | ||||||
| layout?.wideSize ) | ||||||
| ? { ...globalLayoutSettings, ...layout, type: 'constrained' } | ||||||
| : { ...globalLayoutSettings, ...layout, type: 'default' }; | ||||||
|
|
||||||
| // If there is a Post Content block we use its layout for the block list; | ||||||
| // if not, this must be a classic theme, in which case we use the fallback layout. | ||||||
| const blockListLayout = postContentBlock | ||||||
| ? postContentLayout | ||||||
| : fallbackLayout; | ||||||
|
There was a problem hiding this comment. Hi there! I stumbled on this code while doing some performance debugging. While right now, it's not clear to me that this code (I mean all the figuring out of the layout) is introducing performance regression (specially loading) but I noticed that the value of the layout changes three times for a regular post loaded in the post editor.
Thanks There was a problem hiding this comment. We don't have access to the post template straightaway on load, so we need to do a little checking to see when it becomes available. But we should probably be checking whether postContentBlock is an empty object rather than just checking whether it exists, as then The definitions object appeared in #40875; it's where we store base styles for each layout type. When we get the layout settings with
That's a good point, I can't say offhand but it's worth looking into! There was a problem hiding this comment.
What's seems not correct here is that the "definition" are included in the "default layout" object. I'm fine having "definitions" but I don't they should be part of the default layout object. There was a problem hiding this comment.
Yeah, is there a way to have no changes at all, this is to avoid rendering anything if the correct layout object is not available yet. As I guess otherwise, we'd just be doing unnecessary computation and potentially causing layout shifts in the UI. There was a problem hiding this comment.
I'm not sure I understand. The default layout type has default styles too, so we need to access those definitions sometimes. Do you mean we shouldn't be returning the definitions from
Yeah, there will be a layout shift if the layout is anything other than center-aligned and constrained to default theme width. The problem is, if we wait for the post template data to be available before rendering the post content, users will be looking at a white screen for a little bit, which seems to me a worse experience than the layout shift. Unless we can find a faster way to access the Post Content block layout? The thing is we need the Post Content block from the specific template being used with the post, because it might have custom settings. It's not enough to get the data from global styles. There was a problem hiding this comment. Thinking through this a bit, also in light of #45262, would it be terrible to get the Post Content layout and add it to the block editor settings here? I haven't tried it out yet but happy to do so as I think we're going to need a solution that renders the correct layout for all types of users, not just the ones with the right permissions 😬 There was a problem hiding this comment. I think in general we're trying to move away from server-side provided settings in order to make the editor more portable (outside wordpress, other contexts...) but in this case, I wouldn't mind it. The original idea of the "layout" properly in theme.json was to avoid this problem exactly: the layout in theme.json would always correspond (as a convention) to the post content block one and we'd avoid any complex computations. (statically defined) There was a problem hiding this comment. Thanks for the context! Yeah, the issue is precisely that folks wanted to customize the Post Content layout, and with features such as #44065 it makes sense for the post editor to reflect whatever layout is set. |
||||||
|
|
||||||
| const titleRef = useRef(); | ||||||
| useEffect( () => { | ||||||
|
|
@@ -257,13 +335,24 @@ export default function VisualEditor( { styles } ) { | |||||
| { themeSupportsLayout && | ||||||
| ! themeHasDisabledLayoutStyles && | ||||||
| ! isTemplateMode && ( | ||||||
| <> | ||||||
| <LayoutStyle | ||||||
| selector=".edit-post-visual-editor__post-title-wrapper, .block-editor-block-list__layout.is-root-container" | ||||||
| layout={ fallbackLayout } | ||||||
| layoutDefinitions={ | ||||||
| globalLayoutSettings?.definitions | ||||||
| } | ||||||
| /> | ||||||
| { postContentLayoutStyles && ( | ||||||
| <LayoutStyle | ||||||
| layout={ postContentLayout } | ||||||
| css={ postContentLayoutStyles } | ||||||
| layoutDefinitions={ | ||||||
| globalLayoutSettings?.definitions | ||||||
| } | ||||||
| /> | ||||||
| ) } | ||||||
| </> | ||||||
| ) } | ||||||
| { ! isTemplateMode && ( | ||||||
| <div | ||||||
|
|
@@ -288,7 +377,7 @@ export default function VisualEditor( { styles } ) { | |||||
| ? 'wp-site-blocks' | ||||||
| : `${ blockListLayoutClass } wp-block-post-content` // Ensure root level blocks receive default/flow blockGap styling rules. | ||||||
|
There was a problem hiding this comment. I was wondering if we might still need the There was a problem hiding this comment. I'm a bit hazy on how the global styles stuff works in the post editor, so if you think it's needed I'll just add it back. Won't do any harm to have the class there 😄 There was a problem hiding this comment.
I was looking at this the other day while reviewing other PRs, so in case the breadcrumbs here are helpful: it's pretty naive when it comes to global styles. The server-rendered global styles are added to the block editor settings in PHP here, and then the post editor figures out whether or not to use them here. The styles are then passed down to the post editor's The TL;DR is that it's nearly as if the global styles stylesheet had been enqueued, only those styles get transformed a bit by the I mostly just wanted to write that out somewhere because it finally clicked for me how it's hooked together 😀 There was a problem hiding this comment. Thanks, that's really helpful! |
||||||
| } | ||||||
| __experimentalLayout={ blockListLayout } | ||||||
| /> | ||||||
| </RecursionProvider> | ||||||
| </MaybeIframe> | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice abstraction here!