Fix Template Part renaming error. #24500
Conversation
Addison-Stavlo
added
[Type] Bug
Addison-Stavlo
requested review from
ajitbohra,
Soean and
talldan
as code owners
|
Size Change: +4 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
Addison-Stavlo
requested review from
noahtallen,
epiqueras,
ockham,
vindl,
Copons and
jeyip
Addison-Stavlo
added
the
[Status] In Progress
Addison-Stavlo
changed the title
I'm a little confused by this. Is this the scenario?
|
|
Ah I think I understand more testing it out. Now, when you rename it to anything and then save changes, it seems to switch the template part back to whatever it was looking at previously, which causes some UI jankiness, and then it references the old name. But after refreshing, the new name shows up correctly. |
| useEffect( () => { | ||
| setAttributes( { postId } ); | ||
| }, [ postId ] ); |
There was a problem hiding this comment.
This seems to fix it. However, it means that when we first load a template supplied by a theme, the template and all of its template parts will be marked as 'dirty' immediately since they have all been given explicit postIds. I dont think this is necessarily bad behavior, just different than before. 🤔
There was a problem hiding this comment.
interesting. I think this behavior also existed at some point in the past (I think @epiqueras changed that somehow)
There was a problem hiding this comment.
I don't think this behavior is acceptable. We made an effort to remove it because it's confusing.
There was a problem hiding this comment.
We made an effort to remove it because it's confusing.
True, definitely confusing if we are prompting the user to save changes to something they haven't made changes to. 🤔
There was a problem hiding this comment.
Alternative idea - 61fa1bf - set the postId attribute in the name-panel when the slug is changed. This will prevent it from disappearing when renamed, allow the template to dirty to save the proper id for its child, and not give us any issues with dirty loading. 🤔
There was a problem hiding this comment.
I think there needs to be a clear distinction between Template Parts that are being referenced from the theme, and those that are customized by the user. Because once you start editing a Template Part, you're actually editing a newly-created duplicate, not the one from the theme. If your theme updates, the Template Part won't change with it, because you're no longer using the theme's version of the Template Part. But this is never conveyed clearly to the user.
I think theme-provided Template Parts should be in a sort of "read-only" mode by default, with an explicit "Customize/Edit" button that, when pressed, will copy it and create the editable version. Right now it's not clear at all whether a Template Part is still being referenced from a theme or not, and I think this is the source of a lot of the confusing behavior/logic.
There was a problem hiding this comment.
That makes sense to me for sure. Imo, we can move forward with the fixes here, and then come up with a design for that interaction as well.
There was a problem hiding this comment.
If your theme updates, the Template Part won't change with it
I could be wrong, but now that you mention this I think this may still be the case even if the user has not edited the template part? On first use an auto-draft is created from the theme's .html files with the slug name, that is what is being used by the editor. If the auto-draft with that slug/theme already exists, I don't think we create a new one and overwrite it. So if the themes template part .html is updated, the editor would still be looking for the auto-draft by slug which was created with the outdated version. 🤔
I think theme-provided Template Parts should be in a sort of "read-only" mode by default, with an explicit "Customize/Edit" button that, when pressed, will copy it and create the editable version.
I think if we handle it this way for template parts we should probably do the same for templates as well?
There was a problem hiding this comment.
I think if we handle it this way for template parts we should probably do the same for templates as well?
Yes, that sounds like the right behavior to me.
Addison-Stavlo
removed
the
[Status] In Progress
Addison-Stavlo
changed the title
There was a problem hiding this comment.
Nice! This works perfectly to fix the issue in my testing without causing other side effects.
I think we should totally follow up to address @ZebulanStanphill's concerns about template parts coming from themes.
Description
Fixes #24000 by using ensuring the
postIdattribute is set on the block when we update theslug.How has this been tested?
Load a fresh template part supplied by the theme and attempt to rename it.
Verify it does not disappear from the screen.
Verify saving works as expected.
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: