Skip to content

Add layout styles from Post Content block to post editor #44258

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tellthemachines merged 14 commits into from
Sep 27, 2022
Merged

Add layout styles from Post Content block to post editor #44258

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
95cb34a
Add layout styles from Post Content block to post editor
tellthemachines Sep 19, 2022
84cddef
Fix post content finding logic.
tellthemachines Sep 20, 2022
0128131
Remove extra styles when Post Content block exists.
tellthemachines Sep 20, 2022
b8d56b8
templateContent should always be a string
tellthemachines Sep 20, 2022
58a2580
Memoise post content block.
tellthemachines Sep 20, 2022
602c7df
Change function signature and mark experimental
tellthemachines Sep 21, 2022
7102ea3
_Really_ make sure templateContent is a string
tellthemachines Sep 21, 2022
1b29729
Update layout type for legacy layouts.
tellthemachines Sep 21, 2022
08fe809
Fix selector specificity and legacy layout logic.
tellthemachines Sep 21, 2022
eeb03e3
Fix 404s on fetch and only parse content if needed
tellthemachines Sep 23, 2022
f2f7e25
Fix acces to layout when no Post Content block
tellthemachines Sep 26, 2022
67c829d
Add doc comment to findPostContent
tellthemachines Sep 26, 2022
bcdeda8
Increase specificity
tellthemachines Sep 26, 2022
fb51a82
Remove unneeded LayoutStyle
tellthemachines Sep 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Add layout styles from Post Content block to post editor
  • Loading branch information
@tellthemachines
tellthemachines committed Sep 23, 2022
commit 95cb34a54444fb829dfb39f6b37a5aebf58e1462
22 changes: 22 additions & 0 deletions packages/block-editor/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -766,6 +766,28 @@ _Parameters_
- _props_ `Object`: Optional. Props to pass to the element. Must contain the ref if one is defined.
- _options_ `Object`: Optional. Inner blocks options.

### useLayoutClasses

Generates the utility classnames for the given blocks layout attributes.
This method was primarily added to reintroduce classnames that were removed
in the 5.9 release (<https://github.com/WordPress/gutenberg/issues/38719>), rather
than providing an extensive list of all possible layout classes. The plan is to
have the style engine generate a more extensive list of utility classnames which
will then replace this method.
Copy link
Contributor

@andrewserong andrewserong Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feedback is probably low priority, but just jotting down some thoughts:

When this function was originally written, I believe we were thinking that the style engine might be responsible for more of the layout support that we do now (I think this function was written prior to the layout refactor landing). It might be worth us updating the description here, as I believe it probably makes sense for useLayoutClasses to be the entry point for getting layout classnames — if in some point in the future we do use the style engine for constructing the classnames, we might wind up doing it as an internal implementation detail, so we'd still probably want this as the main function for getting those classnames?

In terms of stability / API signature, the function currently accepts the layout object, and a layoutDefinitions object. However, in useLayoutStyles the signature is a bit different in that it accepts a block object and a selector. Would it be worth coming up with a signature for both of these hooks that is pretty much the same, to create some consistency there? I like the use of the block object in the latter — we might wind up needing to access other aspects of the block object in useLayoutClasses in the future, too, so it could be worth making some changes there?

Copy link
Contributor Author

@tellthemachines tellthemachines Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for thinking this through! I'll update the description to remove references to hypothetical future style engine work 😅

Would it be worth coming up with a signature for both of these hooks that is pretty much the same, to create some consistency there?

We wouldn't need the selector for useLayoutClasses though, would we? Other than that I'm happy to change it to take the block object.

Copy link
Contributor

@andrewserong andrewserong Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We wouldn't need the selector for useLayoutClasses though, would we?

Ah, good point. Yeah, I can't think of a circumstance where we'd need the selector for getting the classnames. We might have use for it eventually for where we output the classnames, but not for retrieving them in the first place?

Copy link
Contributor Author

@tellthemachines tellthemachines Sep 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I don't think we'd ever need the selector in useLayoutClasses.

I'm also struggling to think of a use for the whole block in this function; we only really need the layout object. We could potentially change it to get the layout definitions from useSetting( 'layout' ) instead of passing them as a parameter. Would that work everywhere?

In useLayoutStyles we need both layout and styles from attributes, as well as the block name to pass to the layout type's useLayoutStyles function, so it can work out whether the block skips serialisation or not.

It's not looking like we can make the signatures very consistent here 😅

Regarding the doc comment for useLayoutClasses, could we simply shorten it to "Generates the utility classnames for the given blocks layout attributes.", or is there any additional info that might be useful here?

Also, I'm thinking we should make these experimental given Layout itself has that status.

Copy link
Contributor

@andrewserong andrewserong Sep 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we only really need the layout object

Right now that's definitely the case. My only thought was if we wind up eventually needing to use parts of the spacing object for potential future classnames (e.g. if we needed to add a has-gap classname further down the track, which I think was discussed a little in the spacing presets discussions), then it'd be easier if we already had the object there.

We could potentially change it to get the layout definitions from useSetting( 'layout' ) instead of passing them as a parameter. Would that work everywhere?

That very well could. I think passing it around is probably a result of some of these functions originally being pure functions instead of React hooks? Now that they're hooks, it's probably better to keep the API signature simpler instead of passing stuff around 🤔

"Generates the utility classnames for the given blocks layout attributes."

Sounds good to me!

Also, I'm thinking we should make these experimental given Layout itself has that status.

Good idea, there's still plenty that we're moving around, so would be good to maintain that flexibility 👍


_Parameters_

- _layout_ `Object`: Layout object.
- _layoutDefinitions_ `Object`: An object containing layout definitions, stored in theme.json.

_Returns_

- `Array`: Array of CSS classname strings.

### useLayoutStyles

Undocumented declaration.

### useSetting

Hook that retrieves the given setting for the block instance in use.
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/hooks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import './metadata';
import './metadata-name';

export { useCustomSides } from './dimensions';
export { useLayoutClasses, useLayoutStyles } from './layout';
export { getBorderClassesAndStyles, useBorderProps } from './use-border-props';
export { getColorClassesAndStyles, useColorProps } from './use-color-props';
export { getSpacingClassesAndStyles } from './use-spacing-props';
Expand Down
20 changes: 19 additions & 1 deletion packages/block-editor/src/hooks/layout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ const layoutBlockSupportKey = '__experimentalLayout';
*
* @return { Array } Array of CSS classname strings.
*/
function useLayoutClasses( layout, layoutDefinitions ) {
export function useLayoutClasses( layout, layoutDefinitions ) {
const rootPaddingAlignment = useSelect( ( select ) => {
const { getSettings } = select( blockEditorStore );
return getSettings().__experimentalFeatures
Expand Down Expand Up @@ -89,6 +89,24 @@ function useLayoutClasses( layout, layoutDefinitions ) {
return layoutClassnames;
}

export function useLayoutStyles( block = {}, selector ) {
Copy link
Contributor

@andrewserong andrewserong Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice abstraction here!

const { attributes = {}, name } = block;
const { layout = {}, style = {} } = attributes;
const fullLayoutType = getLayoutType( layout?.type || 'default' );
const defaultThemeLayout = useSetting( 'layout' ) || {};
const blockGapSupport = useSetting( 'spacing.blockGap' );
const hasBlockGapSupport = blockGapSupport !== null;
const css = fullLayoutType?.getLayoutStyle?.( {
blockName: name,
selector,
layout,
layoutDefinitions: defaultThemeLayout?.definitions,
style,
hasBlockGapSupport,
} );
return css;
}

function LayoutPanel( { setAttributes, attributes, name: blockName } ) {
const { layout } = attributes;
const defaultThemeLayout = useSetting( 'layout' );
Expand Down
2 changes: 2 additions & 0 deletions packages/block-editor/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ export {
getSpacingClassesAndStyles as __experimentalGetSpacingClassesAndStyles,
getGapCSSValue as __experimentalGetGapCSSValue,
useCachedTruthy,
useLayoutClasses,
useLayoutStyles,
} from './hooks';
export * from './components';
export * from './elements';
Expand Down
4 changes: 3 additions & 1 deletion packages/block-library/src/post-content/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ function EditableContent( { layout, context = {} } ) {
return getSettings()?.supportsLayout;
}, [] );
const defaultLayout = useSetting( 'layout' ) || {};
const usedLayout = !! layout && layout.inherit ? defaultLayout : layout;
const usedLayout = ! layout?.type
? { ...defaultLayout, ...layout, type: 'default' }
: { ...defaultLayout, ...layout };
const [ blocks, onInput, onChange ] = useEntityBlockEditor(
'postType',
postType,
Expand Down
69 changes: 54 additions & 15 deletions packages/edit-post/src/components/visual-editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,16 @@ import {
__unstableUseMouseMoveTypingReset as useMouseMoveTypingReset,
__unstableIframe as Iframe,
__experimentalRecursionProvider as RecursionProvider,
useLayoutClasses,
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
Expand Down Expand Up @@ -82,18 +85,31 @@ function MaybeIframe( {
);
}

function findPostContent( blocks ) {
Copy link
Member

@ramonjd ramonjd Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ) {
return findPostContent( blocks[ i ].innerBlocks );
Copy link
Contributor

@andrewserong andrewserong Sep 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we only want to return here if we've found a post content block? Otherwise it looks like findPostContent bails early without having found something if the first depth-first discovery does not match a post content block.

In my current template, it bails after examining the deepest separator block, rather than moving up a level to eventually find the post content block.

In the below screenshot, I logged out looking at for each instance of the for loop, and it appears to stop at the core/separator, where it should have moved up a level and found core/spacer and then moved down one to core/post-content.

image

So, maybe something like the following might work?

let foundPostContent;

...

foundPostContent = findPostContent( blocks[ i ].innerBlocks );
if ( foundPostContent ) {
    return foundPostContent;
}

Copy link
Contributor Author

@tellthemachines tellthemachines Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhhh well spotted! Yeah it'll have to be something along those lines.

}
}
}

export default function VisualEditor( { styles } ) {
const {
deviceType,
isWelcomeGuideVisible,
isTemplateMode,
templateContent = '',
wrapperBlockName,
wrapperUniqueId,
} = useSelect( ( select ) => {
const {
isFeatureActive,
isEditingTemplate,
__experimentalGetPreviewDeviceType,
getEditedPostTemplate,
} = select( editPostStore );
const { getCurrentPostId, getCurrentPostType } = select( editorStore );
const _isTemplateMode = isEditingTemplate();
Expand All @@ -109,6 +125,7 @@ export default function VisualEditor( { styles } ) {
deviceType: __experimentalGetPreviewDeviceType(),
isWelcomeGuideVisible: isFeatureActive( 'welcomeGuide' ),
isTemplateMode: _isTemplateMode,
templateContent: getEditedPostTemplate()?.content,
wrapperBlockName: _wrapperBlockName,
wrapperUniqueId: getCurrentPostId(),
};
Expand All @@ -122,16 +139,13 @@ export default function VisualEditor( { styles } ) {
themeHasDisabledLayoutStyles,
themeSupportsLayout,
assets,
useRootPaddingAwareAlignments,
isFocusMode,
} = useSelect( ( select ) => {
const _settings = select( blockEditorStore ).getSettings();
return {
themeHasDisabledLayoutStyles: _settings.disableLayoutStyles,
themeSupportsLayout: _settings.supportsLayout,
assets: _settings.__unstableResolvedAssets,
useRootPaddingAwareAlignments:
_settings.__experimentalFeatures?.useRootPaddingAwareAlignments,
isFocusMode: _settings.focusMode,
};
}, [] );
Expand Down Expand Up @@ -197,11 +211,25 @@ export default function VisualEditor( { styles } ) {
return { type: 'default' };
}, [ isTemplateMode, themeSupportsLayout, defaultLayout ] );

const blockListLayoutClass = classnames( {
'is-layout-constrained': themeSupportsLayout,
'is-layout-flow': ! themeSupportsLayout,
'has-global-padding': useRootPaddingAwareAlignments,
} );
const templateBlocks = parse( templateContent );
const postContentBlock = findPostContent( templateBlocks );
const postContentLayoutClasses = useLayoutClasses(
postContentBlock?.attributes?.layout,
defaultLayout?.definitions
);

const blockListLayoutClass = classnames(
{
'is-layout-flow': ! themeSupportsLayout,
'wp-container-visual-editor': themeSupportsLayout,
},
postContentLayoutClasses
);

const postContentLayoutStyles = useLayoutStyles(
postContentBlock,
'.wp-container-visual-editor'
Copy link
Contributor

@andrewserong andrewserong Sep 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this selector needs to be a higher specificity selector such as .block-editor-block-list__layout.is-root-container, which I think was used prior to this PR? If I use that one on this line, the layout styles appear to win out over the TwentyTwentyTwo styles as expected. But I wasn't sure if you changed this selector for another reason?

Current PR (TwentyTwentyTwo's margins override the left content justification) When updating to the higher specificity selector
image image

Copy link
Contributor Author

@tellthemachines tellthemachines Sep 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really! Must be my subconscious drive to lower specificity everywhere 😅
It's a good idea to use the selector we had before though.

);
Copy link
Contributor

@andrewserong andrewserong Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we ultimately want to wrap this in a useMemo so that we only run it when templateContent changes?

Copy link
Contributor Author

@tellthemachines tellthemachines Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! I never know when it's best to useMemo or not 😅 Another option would be to return null from useLayoutStyles if there's no block.

Copy link
Contributor Author

@tellthemachines tellthemachines Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option would be to return null from useLayoutStyles if there's no block.

Actually I don't think we can do that because it's using hooks underneath 😕

Copy link
Contributor

@andrewserong andrewserong Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never know when it's best to useMemo or not 😅

Me either, it's often a bit tricky to work out when they're useful. In principle if we're doing potentially expensive things like block parsing on a component that might get rendered multiple times, I think it makes sense to add a useMemo. If I add a wrapper findPostContentWrapper function that calls findPostContent and log out the number of times it's called, in my local environment it looks like we call it 9 times (not sure how valid that is, since there's only 5 log lines there, but it at least suggests we might benefit from the useMemo caching 🤔):

image

From memory we can't call a hook from within another hook, so the optimisation might only be to wrap the block containing the two lines parse and findPostContent in a useMemo, so that we only parse / find blocks when the template content changes?

But this can totally be something to look at in the final stages of the PR.

Copy link
Contributor Author

@tellthemachines tellthemachines Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I tried what you recommended and it seems to be working well! We can still see a jump in layout when the page first loads, I think because the initial data from useSelect takes a while to appear, but I don't think there's much to be done about that.

Copy link
Contributor

@andrewserong andrewserong Sep 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think because the initial data from useSelect takes a while to appear, but I don't think there's much to be done about that.

Probably outside the scope of this PR, but I wonder if there's a way to preload the data for this selector? I see that the Navigation block has a preloading function here and sets apiFetch to use it here. There's similar logic in gutenberg_initialise_editor here, so I wonder if there's a PHP filter of some kind to add in a desired API endpoint to pre-fetch or something? Might be a bit of a rabbithole 😅

I don't at all understand the logic there, but just in case something along those lines sparks any ideas 🙂


const titleRef = useRef();
useEffect( () => {
Expand Down Expand Up @@ -257,13 +285,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={ layout }
layoutDefinitions={
defaultLayout?.definitions
}
/>
<>
<LayoutStyle
selector=".edit-post-visual-editor__post-title-wrapper, .block-editor-block-list__layout.is-root-container"
layout={ layout }
layoutDefinitions={
defaultLayout?.definitions
}
/>
{ postContentLayoutStyles && (
<LayoutStyle
layout={ layout }
css={ postContentLayoutStyles }
layoutDefinitions={
defaultLayout?.definitions
}
/>
) }
</>
) }
{ ! isTemplateMode && (
<div
Expand Down