Full Site Editing: Add Support for Templates Default and Custom Titles and Descriptions (JS Side)#27038
Conversation
Copons
requested review from
Addison-Stavlo,
david-szabo97,
gziolo,
jeyip,
mattwiebe,
noahtallen and
vindl
Copons
changed the title
|
Size Change: +660 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
Copons
force-pushed
the
update/default-template-types
branch
from
4fbac9c to
8c907a6
Compare
noahtallen
left a comment
noahtallen
left a comment
There was a problem hiding this comment.
looks pretty solid to me, just left a few comments
| key | ||
| ); | ||
| const { title: templateTitle } = select( | ||
| 'core/edit-site' |
There was a problem hiding this comment.
This feels really weird to me -- are we sure this package can rely on the edit site store being available all the time? I wonder if there are scenarios where it could be used when the store hasn't been registered
There was a problem hiding this comment.
There is an early return for ! select( 'core/edit-site' ) a few lines above. 🙂
Anyhow, I'm not happy with this as well.
I would absolutely love to move the getTemplateInfo selector elsewhere shared (e.g. core/editor).
However, there is an inconsistency between edit-post and edit-site initialization that prevent this, as settings initialized in PHP are fed to core/editor in edit-post, and core/edit-site in edit-site.
My opinion is that we should reduce/remove all inconsistencies between the different editor inits, but this is not the place.
I should probably add a todo comment explaining what happens here.
There was a problem hiding this comment.
It creates a cyclic dependency between edit-site and editor that should be avoided. We have just landed API extensions to @wordpress/data (#26655) that is going to prevent this type of reference by using direct store definition imports. Would it be possible to pass entityRecordTitle as part of the record.title prop passed to EntityRecordItem instead or something along those lines?
There was a problem hiding this comment.
This originates from 2 different entities quirks:
auto-draftrecords have their title wiped out when added to the state.wp_template(and parts) records titles fall back to the slug instead of the id.
What we want is for templates to use the default title whenever possible.
This normally would mean to create them with the default title (which is what we do), but then when the template becomes an entity, given it is an auto-draft, it loses the title.
This in turn requires us to have that getTemplateInfo selector which gives templates back their intended title (or handles the various fallbacks).
The EntityRecordItem is, as far as I can see, the only place that would need this "title fix", as it's not tied to any specific entity.
But its part of the editor package, which shouldn't need edit-site.
The post editor initializes its settings into core/editor, while the Site Editor into core/edit-site. I really don't like it, but it's how it is right now.
In one of my previous attempts, I tried adding an exception to the entities actions: if it's a wp_template, don't clear the title.
But that was a bit smelly, and eventually I decided to avoid introducing exceptions in such a key part of the entire app, without understanding all the possible consequences.
The only other option (as far as I can see) it's to handle it whenever needed, which... is where we are right now! 😄
Looking at EntityRecordItem, I don't see a good way to pass the "generated" template title without introducing exceptions to the dirty entities, which is something I'd rather avoid.
On the other hand, I'm exploring what it would take to update the Site Editor init to register the default template types to the core/editor settings instead of the core/edit-site's, removing its unnecessary dependency.
There was a problem hiding this comment.
I've pushed an experimental commit to demonstrate what I mean with "moving defaultTemplateTypes to core/editor: 827464c
Specifically this is where it happens:
827464c#diff-53f39da7a5f1f996f5359a2dd7b2747fb3d60a494523dd73dba946fab8e82592R115-R119
There was a problem hiding this comment.
On the other hand, I'm exploring what it would take to update the Site Editor init to register the default template types to the core/editor settings instead of the core/edit-site's, removing its unnecessary dependency.
It's not optimal and it's hacky. But I don't think we have a better option due to the way of how different editors initialized. ✅
gziolo
left a comment
gziolo
left a comment
There was a problem hiding this comment.
This PR probably requires a rebase on top of update/default-template-types as it includes all changes applied there.
Copons
force-pushed
the
update/default-template-types
branch
from
8c907a6 to
7eef161
Compare
Copons
force-pushed
the
update/use-default-template-types
branch
from
f7f7527 to
689eccb
Compare
Copons
requested review from
ajitbohra,
chrisvanpatten and
mkaz
as code owners
Copons
force-pushed
the
update/default-template-types
branch
from
7eef161 to
61eae5b
Compare
Copons
force-pushed
the
update/use-default-template-types
branch
from
827464c to
99bf728
Compare
Copons
force-pushed
the
update/use-default-template-types
branch
from
99bf728 to
226ab3c
Compare
david-szabo97
left a comment
david-szabo97
left a comment
There was a problem hiding this comment.
Works well and tests are passing! ✅
Copons
force-pushed
the
update/use-default-template-types
branch
from
226ab3c to
e7f9af4
Compare
|
|
||
| - `string`: Post type. | ||
|
|
||
| <a name="getDefaultTemplateType" href="#getDefaultTemplateType">#</a> **getDefaultTemplateType** |
There was a problem hiding this comment.
Should all three new API methods be marked unstable or even experimental to give them some time to mature? This way they won't be exposed in docs until we are 100% sure that we can support them indefinitely 😃
@adamziel has recently extracted store handling related to reusable blocks to its own store and moved to an isolated package. It's something that should be discussed in this context as well. I'd like to get some feedback from @youknowriad who has the best overview how editor packages are architected.
There was a problem hiding this comment.
Yes, you are totally right.
These selectors originated in edit-site where we work under the assumption that everything is experimental, and didn't think of explicitly marking them as such when moving them here.
I'll definitely mark them as __experimental if we decide that this approach is the right one!
There was a problem hiding this comment.
Eh, I went ahead and done it anyway. 😄
TIL about the reusable-blocks package!
It sounds like a very neat idea, that in our case could be used to store all FSE-related functions that might be used in different editors and contexts.
As it always is, naming is hard!
templates sounds clean and nifty, but FSE is not just templates (there are also... template parts! 😄 )
full-site-editing would be catch-all, but it sounds slightly awkward to me.
Also, I've never created a new package.
Is there anything I should know to make it work smoothly?
How would publishing it to npm work?
There was a problem hiding this comment.
This should help with a new package:
https://github.com/WordPress/gutenberg/tree/master/packages#creating-a-new-package
You can wait a bit more with that step. It might help to better understand what scope such package would have. We have also @wordpress/media-utils that is catch it all for media related code 😄
Publishing is automated, so you don't have to worry about it other than set some version lower than 1.0 to make it clear that it's all experimental.
I'm checking #25859 to learn if there is anything that isn't included in the docs but I think everything else will catch CI. I assume you will have to run npm run docs:build to expose README in the documentation portal at https://developer.wordpress.org/block-editor/packages/. It should add a section like this in the manifest file:
Lines 1670 to 1675 in 3aeb89e
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, it's fine to explore and keep the branch ready for the package extraction 😄
gziolo
left a comment
gziolo
left a comment
There was a problem hiding this comment.
Thank you for applying updates 👍
|
I know it's been a long time but I wanted to share some thoughts here as I stumbled on this code :) This PR added a dependency from edit-site to editor package and from block-library to editor package. If I'm understanding properly, this requirement is because we want to add "configuration" (default template parts) to the template part block and that block is useful in both the post editor (template mode) and the site editor so it had been decided that the "editor" package would be the common denominator here to add "block configuration". The issue conceptually for me is that the "editor" package is not meant as a package to hold block configurations, it's actually a package to hold reusable components to build "post editors" (not even site editors), basically the edit-site should ideally not depend on it at all. I think it's probably fine for now as is, but I guess the ideal scenario is to have a |
5 participants
Description
This PR was originally part of #26636 and it depends on #27036.
draft. They will becomepublishedonce saved the first time.getTemplateInfoutility into thegetTemplateInfoselector, since it relies on thecore/editorstate.How has this been tested?
These test steps take advantage of #27034.
If it's not merged yet, just skip the steps related to the "wp-admin Templates list".
wp_templateitems of statusauto-draft; there might be duplicates for unrelated reasons./edit.php?post_type=wp_template).auto-drafttemplates. Feel free to wipe this list clean by moving all templates to trash, and permanently delete them.+in the Templates menu).Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: