Block Library: Implement Template Part block editing. #18925
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,118 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { | ||
| useEntityProp, | ||
| __experimentalUseEntitySaving, | ||
| EntityProvider, | ||
| } from '@wordpress/core-data'; | ||
| import { useMemo, useCallback } from '@wordpress/element'; | ||
| import { parse } from '@wordpress/blocks'; | ||
| import { serializeBlocks } from '@wordpress/editor'; | ||
| import { Button } from '@wordpress/components'; | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { InnerBlocks } from '@wordpress/block-editor'; | ||
| import { useSelect } from '@wordpress/data'; | ||
|
|
||
| const saveProps = [ 'content', 'status' ]; | ||
| function TemplatePart() { | ||
| const [ content, _setContent ] = useEntityProp( | ||
| 'postType', | ||
| 'wp_template_part', | ||
| 'content' | ||
| ); | ||
| const [ status, setStatus ] = useEntityProp( | ||
| 'postType', | ||
| 'wp_template_part', | ||
| 'status' | ||
| ); | ||
| const initialBlocks = useMemo( () => { | ||
| if ( status !== 'publish' ) { | ||
| // Publish if still an auto-draft. | ||
| setStatus( 'publish' ); | ||
| } | ||
| if ( typeof content !== 'function' ) { | ||
| const parsedContent = parse( content ); | ||
| return parsedContent.length ? parsedContent : undefined; | ||
| } | ||
| }, [] ); | ||
| const [ blocks = initialBlocks, setBlocks ] = useEntityProp( | ||
| 'postType', | ||
| 'wp_template_part', | ||
| 'blocks' | ||
| ); | ||
| const [ isDirty, isSaving, save ] = __experimentalUseEntitySaving( | ||
| 'postType', | ||
| 'wp_template_part', | ||
| saveProps | ||
| ); | ||
| const saveContent = useCallback( () => { | ||
| _setContent( content( { blocks } ) ); | ||
| save(); | ||
| }, [ content, blocks ] ); | ||
| const setContent = useCallback( () => { | ||
| _setContent( ( { blocks: blocksForSerialization = [] } ) => | ||
| serializeBlocks( blocksForSerialization ) | ||
|
There was a problem hiding this comment. Is there not an equivalent selector that grants us access to the serialization of these blocks, rather than exposing the internal utility functions? Would it be possible to implement such a selector, or enhancement to an existing selector? I'm not entirely clear what it is about the editor's serialization that we want to reuse here, vs. the raw There was a problem hiding this comment. There is no selector that just serializes the inner blocks of a given block. We could extend We need this util from the editor, for the backwards compatibility reasons explained in the file: I think this utility could be moved to There was a problem hiding this comment. I think those behaviors might still be necessary for the post editor, but I don't know that they would need to extend to templates. The main purpose for the post editor is:
I don't think either of these are really relevant for the template parts though? To me, it seems like maybe we can keep that utility for default usage, then just use the block module's default There was a problem hiding this comment. Perfect, we can just use |
||
| ); | ||
| }, [] ); | ||
| return ( | ||
| <> | ||
| <Button | ||
| isPrimary | ||
| className="wp-block-template-part__save-button" | ||
| disabled={ ! isDirty || ! content } | ||
| isBusy={ isSaving } | ||
| onClick={ saveContent } | ||
| > | ||
| { __( 'Save' ) } | ||
| </Button> | ||
| <InnerBlocks value={ blocks } onChange={ setBlocks } onInput={ setContent } /> | ||
|
There was a problem hiding this comment. These new props Or, alternatively, based on what we're doing above with serializing blocks and how it's proposed to expose the internal state utilities, maybe this is something which should be baked into For that, I might be okay with an Then again, that seems like something we could also achieve without a callback, and instead using the To me, this is a very similar problem to what was previously discussed in #5596 around having a callback to There was a problem hiding this comment.
We don't want a split block list. We want the child blocks to still save to the parent through the block's This change provides a way to provide an initial value and then sync with the changes of We could achieve something similar with actions and selectors, albeit in a much more verbose and error-prone way. We would probably end up implementing a custom hook for it and the logic for checking if the last check was persistent might get messy. This approach would also be less performant. I don't think this is comparable to a callback for What are the issues you see this creating in the future? There was a problem hiding this comment.
I think the issue for me is that we've now introduced (to me at least) some confusion around exactly what For me, this sort of controlled rendering was was Maybe if we compare it to React's distinction of controlled vs. uncontrolled inputs, it's fine to consider support for both use-cases. I think there's a bit of a difference in how what would become the equivalent of an "uncontrolled"
To a lesser degree, I still think we'll have some concern about how difficult it is to manage this value from a block. For example, with this implementation, are we serializing blocks on every keypress within a template? That seems like it could become a performance concern for non-trivial templates. Whether that's something we could improve, it would be fair to acknowledge that others may encounter similar pain points. I'm not sure it's something we need to act on now, but it gets me me wondering whether there are alternative / additional abstractions we could imagine to help encapsulate this behavior of mapping the blocks data to a custom entity serialization. As before I was talking of this sort of InnerBlocks being an enhanced version of a BlockEditorProvider, I wonder if there's some parallel of how Editor is an enhanced version of BlockEditor responsible for managing how a block editor saves to a post. There was a problem hiding this comment.
That's fair. I agree we should avoid it if possible.
Yes, that's what I based it on.
We're not serializing on every keypress, but I also see this becoming a common pattern and it would be good to deal with all the pain points in one place.
We could have an enhanced |
||
| </> | ||
| ); | ||
| } | ||
|
|
||
| export default function TemplatePartEdit( { | ||
| attributes: { postId: _postId, slug, theme }, | ||
| setAttributes, | ||
| } ) { | ||
| const postId = useSelect( | ||
| ( select ) => { | ||
| if ( _postId ) { | ||
| // This is already a custom template part, | ||
| // use its CPT post. | ||
| return ( | ||
| select( 'core' ).getEntityRecord( | ||
| 'postType', | ||
| 'wp_template_part', | ||
| _postId | ||
| ) && _postId | ||
| ); | ||
| } | ||
|
|
||
| // This is not a custom template part, | ||
| // load the auto-draft created from the | ||
| // relevant file. | ||
| const posts = select( 'core' ).getEntityRecords( | ||
| 'postType', | ||
| 'wp_template_part', | ||
| { | ||
| status: 'auto-draft', | ||
| slug, | ||
| meta: { theme }, | ||
| } | ||
| ); | ||
| if ( posts && posts[ 0 ].id ) { | ||
| // Set the post ID so that edits | ||
| // persist. | ||
| setAttributes( { postId: posts[ 0 ].id } ); | ||
|
There was a problem hiding this comment. I would not expect this mapping function to have a side effect. There was a problem hiding this comment. What's the problem with it? There was a problem hiding this comment.
One of the major goals of a getter / setter pairing of For me, it's largely a matter of maintenance overhead and potential for bugs introduced as part of said maintenance. There's otherwise nothing technically incorrect about this implementation. There was a problem hiding this comment.
Good point, I'll change it. |
||
| return posts[ 0 ].id; | ||
| } | ||
| }, | ||
| [ _postId, slug, theme ] | ||
| ); | ||
| return postId ? ( | ||
| <EntityProvider kind="postType" type="wp_template_part" id={ postId }> | ||
| <TemplatePart /> | ||
| </EntityProvider> | ||
| ) : null; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| .wp-block-template-part__save-button { | ||
| position: absolute; | ||
| right: 0; | ||
| top: 0; | ||
| z-index: z-index(".wp-block-template-part__save-button"); | ||
| } |
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.
In the above
useMemo, we test whethercontentis a function. Here, we have no such safety checks. Do we run any risk that this isn't a function?Aside: I'm not really clear why this is a function.
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.
useMemoruns on the first render.InnerBlockswill set it to a function after that.It mirrors how the editor store works. It uses a function that computes the eventual value to make the entity dirty in the store without having to run a potentially expensive computation.
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.
Is this behavior documented somewhere? Could we at least include an inline comment to explain this?
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.
I'm refactoring this into a custom hook with docs, because it will be used often,
useSyncedEntityInnerBlocks.