Edit Site: Add convert to template part flow. #20445
Conversation
epiqueras
requested review from
ajitbohra,
ellatrix,
jorgefilipecosta,
Soean,
talldan and
youknowriad
as code owners
|
Size Change: +253 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
|
This is great. It starts to tie in experiences that were laid back with reusable blocks. I expect this to be a very common way to build a theme or template — start by designing with blocks, then group the pieces that make sense for different template parts. |
epiqueras
requested review from
MichaelArestad,
noahtallen,
vindl and
Addison-Stavlo
| @@ -14,7 +14,8 @@ export default function TemplatePartInnerBlocks() { | |||
| ); | |||
| return ( | |||
| <InnerBlocks | |||
| __experimentalBlocks={ blocks } | |||
| // Allow initial blocks from a block transform to load, if any. | |||
| __experimentalBlocks={ blocks.length === 0 ? undefined : blocks } | |||
There was a problem hiding this comment.
this should be value={ } now :) I'm also not sure if it's still needed with the latest block sync changes
packages/blocks/README.md
Outdated
| @@ -730,6 +730,7 @@ _Parameters_ | |||
|
|
|||
| - _blocks_ `(Array|Object)`: Blocks array or block object. | |||
| - _name_ `string`: Block name. | |||
| - _isGroupingBlock_ `boolean`: Set to true to bypass default grouping block selection. | |||
There was a problem hiding this comment.
What does this do? Is bock transform normally wrapped by a group block or something?
| import { switchToBlockType } from '@wordpress/blocks'; | ||
| import { __ } from '@wordpress/i18n'; | ||
|
|
||
| export default function TemplatePartConverter() { |
There was a problem hiding this comment.
Should this live in the block-editor package? It seems like it would fit well there, since it's basically part of the block menu. (maybe we could then provide a mechanism to turn it off for the post editor?)
There was a problem hiding this comment.
I would assume that if we want this in both editors (site and post) when FSE is enabled, that this would be better in block-editor as well. However, if it doesn't belong in the post editor I would think this is a fitting location for it? (asked about this below)
There was a problem hiding this comment.
It looks like the idea is to keep it edit-site only (for now at least). Id say edit-site package makes sense, and we can move it over to block-editor if things change and we want this in the post editor as well.
|
I dig this. We can change the text from "template part" to whatever we decide to call them. I know I'm leaning toward "Section". |
|
@MichaelArestad would we still call it "section" if it's something like just a navigation menu, or a list of social icons? |
Good question. I think section still works in those scenarios. |
|
This convert-to flow seems like it will be super-smooth when combined with some of the other creation changes being developed in #23295! Once these flows combine, will we be able to 'undo' the conversion if necessary (and also deleting the auto-draft created for the 'Untitled Section' in that undo) ? @matias mentioned:
It sounds like we have pretty much settled on the term 'Section' ? If thats what we have decided to move forward with then I don't expect to change anyones mind, but I do have some fair concerns regarding this name:
For these reasons I feel that renaming this 'Section' in the editor will cause a handful of different confusion points down the road and be looked back upon as an unfortunate naming convention. |
These are all good points. What do you propose? |
TLDR: To not rename it. Consider a standard user who we are concerned with exposing the term 'Template Part' to:
If a user needs to find the "Template Part" block, it will be to add it to a Template. In this case the term 'Template Part' makes sense. If a user is editing Templates in this manner they are now leaning towards the side of theme development, in which case they should be exposed to the proper term that Themes use. (This assumes we wouldn't use the template part block in the post editor, but I believe this was suggested by both yourself and @mtias in the past? As noted we do have "Reusable Blocks" for that). I personally don't think 'Template Part' really needs to be renamed. It seems like it would cause more confusion than clarity. I really have tried to think of a "good alternative" for the sake of the discussions but I don't feel that there really is any good alternative where the pros outweigh the cons. If it really needs to be renamed, something other than 'Section' would cut out a couple of the confusion points (but not all of them). |
|
@Addison-Stavlo I'm sold on going back to Template Parts. You make excellent points with benefits that outweigh the reasons I wanted to rename it in the first place. Should I create an issue or would you be interested in changing it back? |
Go ahead and create an Issue, I'm stuck on other priorities/duties until the end of next week. It should be a fairly quick task, but if its still around by then I can get to it. |
Addison-Stavlo
force-pushed
the
add/convert-to-template-part-button
branch
from
927b609
to
07fce86
Compare
| useEffect( () => { | ||
| if ( innerBlocks.length ) { | ||
| onCreate(); | ||
| } | ||
| }, [] ); | ||
| if ( innerBlocks.length ) { | ||
| return <Spinner />; | ||
| } |
There was a problem hiding this comment.
Since the placeholder has updated a lot since this PR was originally made, I had to make some changes for this transform to work properly. Here, we pass down the innerBlocks to the placeholder and immediately create the entity if they exist. The idea is that this block will never render the placeholder when innerBlocks are present, unless we are in the conversion flow.
There was a problem hiding this comment.
This PR should be in a working state again:
I am noticing this PR adds the ability to convert to template part in 2 separate areas of the toolbar, the new button as well as the standard block-transform tool.
In transform menu:
New button:
@shaunandrews @jameskoster @MichaelArestad - Are we fine with keeping it in the transform tool and getting rid of the separate button/component? Or do we want to somehow hide it from the transform tool and only allow it to happen from this new button area? (The transform tool seems to appear auto-magically once the transform is defined, which the separate button still relies on for functionality. So I am not certain the best way to hide it if we don't approve of it in the standard transform location). 🤔
|
We're talking about moving the convert template part flow business logic and the placeholder onCreate callback into its parent. You asked a number of great questions in this discussion. To frame my reasoning, I'll start by describing why I thought it could be beneficial to move
In this context, when I was using the word dependency, I was trying to say that I was thinking that if we moved We already see a lot of business logic being consolidated in I favored having onCreate in the parent for these reasons. It could be used in the convert flow if the logic were moved into Hopefully that makes, sense, but let me know if it doesn't. Earlier I mentioned exploring whether or not some of these approaches were even worthwhile to implement. +1 from me whenever we resolve these last few questions. I'll check this thread first thing tomorrow! |
|
Re: Creating the entity in the converter component vs. template part block:
Right, we could use state to track this information, however it would require extra complexity and reworking how the template part block works to enable this in a meaningful way that still allows the standard select/create flows to work and appear when expected vs. the 'loading spinner' that is used to represent waiting a creation that was triggered elsewhere. While reusable blocks is our closest equivalent to model this flow after, there are enough differences in the overall functionalities that we shouldn't expect the underlying code for this flow to look the same. The block controlling reusable blocks has no concept of creation/selection, while the template part block does. Since it does, I think it makes much more sense to keep using this functionality to create the entity after block conversion than to define a new separate creation system elsewhere and work to add extra logic to determine whether the template part block was initiated via a conversion awaiting entity creation elsewhere vs. being added from the inserter. The block shouldn't care about how it was put on the page, it should just initialize and make its determinations in how to function without needing that extra information. The state of this PR back in February did this sort of flow as well. It converted block(s) to innerBlocks of a template part block that represented no entity, the template part block mounted and found no corresponding saved entity and therefore loaded the placeholder, the placeholder was then used to give the slug/theme names in an input and click 'create', and creation was done through the placeholder using the innerBlocks that were present. I think this still makes sense today, the only difference here is that the placeholder no longer asks for slug/theme inputs in creating a new template part as it was updated to create all new custom template parts as 'Untitled Template Part'. Since that creation flow was simplified, this PR has been updated to trigger that same creation from that same place when it is necessary. Re: Moving onCreate/innerBlocks out of the placeholder:
I feel the concerns are separated in this way. When the template part block cannot find an entity, the placeholder is rendered and handles what needs to happen for the creation of that entity. Moving some of that creation to one place and leaving the rest of it in another seems like over-separation. That is, we are splitting the same concern up into multiple places instead of having it consolidated into one.
Right, basically I see the current state of that edit file as:
Agreed, there is a lot of business logic there. But I don't see that as a reason to clutter it with more logic if it's not necessary. If the template part block mounts with a corresponding entity it has no need of having an
We could do that, but to me it seems more cluttered and less clearly defined. |
Gotcha 👍 That's the sense I got from previous comments, but I wanted to confirm to make sure that I wasn't misunderstanding. Thanks! |
The point about a block not caring about how it was put on a page is very interesting. I didn't realize that's a typical expectation. I think I'm tracking. Correct me if I'm wrong, but you're saying that the block should act as its own self-contained unit, and when we say make its determinations of how to function, that includes triggering template part creation. |
I think it is preferable, if the block is set up to handle these determinations I don't see a reason to complicate it by having its functionality dependent on more information about how it was added to the editor. |
I see. Enrique created this a while ago, had a certain intention when structuring the PR the way he did, was also handling the convert flow in the placeholder, and was, in fact, requiring even more information than it is now (slug + theme inputs). |
|
Yeah, thats how I see it.
To be clear, the 'convert flow' is and has been happening outside of the block. It is what replaces the blocks with the template part block in the first place. By that point the blocks are technically converted, but the template part block and its corresponding inner blocks that now exist don't correspond to any entity, and that entity creation is still handled by the placeholder. |
|
Just another thought - I think the creation from innerBlocks could add a bit of a protection/fallback as well in some cases. Perhaps there is a template part block with inner blocks in the content passed into the editor that does not have a corresponding entity (maybe it was defined in markup in an unconventional way with no actual corresponding file, or maybe some other circumstance). In that case, instead of the block failing and either loading the placeholder or the final unresolved file spinner, it would automatically trigger creation of 'Untitled Template Part' and the content would still be available as expected. 🤔 |
I think I see what you're conveying here. Correct me if I'm wrong, but we're saying that the concerns that we're separating are:
This is where I disagree, but at the same time, I want to acknowledge that I might frame the concerns in separation of concerns differently because of past experiences at other companies (which may not translate to Automattic). To me it feels more natural to consolidate all business logic in the parent, but the more I think about it, the more I see my disagreements may also be a matter of opinion. Ultimately, the functionality in this PR works well, I believe I understand your reasoning more clearly (chime in if you feel like I'm missing anything big! 🙏 ), and all of my questions have been resolved, so +1 from me 🚀 |
Oh also an interesting point! Giving this some thought 🤔 |
Thats the way I see how the concerns should be separated:
I think what you are proposing would be more:
I think both are valid and will work, and we have a different opinion on which makes more sense? |
Yeah! I think that's a good way to describe this. I agree that both ways will work 👍 It's a difference of opinion, and I definitely don't think that should stop this PR from being merged. |
Addison-Stavlo
force-pushed
the
add/convert-to-template-part-button
branch
from
e1ee1c1
to
50039d7
Compare
|
Thanks for pushing this one through! |
Description
This PR makes it possible to select and convert blocks to a template part in the site editor, similar to how it works for the group and reusable blocks.
It also fixes the breakage that G2 introduced to the block toolbar within the site editor.
How has this been tested?
It was verified that selecting blocks in the site editor, clicking on the block toolbar's settings dropdown, and selecting the option to make a template part, creates a template part with the chosen blocks after inserting the new template part's name and theme.
Screenshots
Types of Changes
New Feature: The site editor now has a button in the block toolbar's settings dropdown for converting a selection of blocks into a template part.
Checklist: