Site Editor: Revert custom templates to their original theme-provided files#28141
Conversation
|
Size Change: +746 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
bb315ee to
6be7a60
Compare
jameskoster
left a comment
There was a problem hiding this comment.
It works!
On the design side my only suggestion would be to use the MenuItem component so that the styles match other popover buttons. Currently there's a mismatch:
| Ellipsis menu | Revert button |
|---|---|
 |
 |
|
Thanks @Copons! I adjusted the language slightly. One last thing – and I suspect you'll want to do this in a separate PR – would it be possible for the Snackbar confirmation to include an undo action to... revert the revert? :D |
Another follow-up enhancement that would either compliment this or maybe make it less necessary would be to have a preview of what we will be reverting to. (any thoughts on that @jameskoster ?) I think that might require a new endpoint to retrieve the file version now that theme//slug is unique. |
Addison-Stavlo
left a comment
There was a problem hiding this comment.
Tested this out and it is working well!
|
Another ❓ - Should this 'revert' flow be possible for bootstrapped custom template parts? Say if I add a template through the "+" sidebar and customize it, should we also have the 'revert' option to put it back to that initial creation state? |
🙇♂️
@jameskoster Let's keep it for a follow up.
@Addison-Stavlo I guess this should be done from the "front-end" side, rather than from Redux. 🤔 Or it might even be better to just delete the template altogether. 🤔 |
Yeah that could be an option, but if we implement the undo affordance then maybe we don't need it? People can just revert and undo.
Good question. Since these templates are not attached to the theme I don't think they should be revertable. They need to be deletable though, which will allow folks to pseudo-revert by deleting one and adding it again :) |
This is a general problem throughout the Site Editor. #24359 Easily reproducible with a single paragraph. Screen.Recording.2021-03-29.at.15.17.23.mov |
|
Ah, I'd never noticed. We can probably address that separately, then. |
jameskoster
left a comment
There was a problem hiding this comment.
Approving the design.
| return new WP_Error( 'rest_template_not_found', __( 'No templates exist with that id.', 'gutenberg' ), array( 'status' => 404 ) ); | ||
| } | ||
|
|
||
| if ( isset( $request['reverted_file_content'] ) && isset( $request['content'] ) && $request['reverted_file_content'] === $request['content'] ) { |
There was a problem hiding this comment.
I'm not quite sure why we need to compare the template content. 🤔
Couldn't we just rely on isset( $request['is_custom'] ) && 'false' === $request['is_custom'] to determine that this is a revert call?
| return; | ||
| } | ||
| // Refetch the template entity | ||
| yield controls.resolveSelect( |
There was a problem hiding this comment.
caused problems in specific situations
Are these problems happening during the apiFetch, or later in the revert, when handling the file template content?
Behind the scenes, getEntityRecord also uses apiFetch, sending the result over to receiveEntityRecords, which in turns dispatches receiveItems which upserts the state.
Throwing a random idea: maybe the problems you encountered were caused by the path?
Have you tried building it in the same way as getEntityRecord?
gutenberg/packages/core-data/src/resolvers.js
Lines 113 to 116 in 4b8b2d9
I think it should turn out to roughly be something like:
const path = addQueryArgs( `/wp/v2/templates/${ template.id }`, {
is_custom: false,
context: 'edit',
} );
Addison-Stavlo
left a comment
There was a problem hiding this comment.
This is looking pretty good to me! Single commented on a couple of the open discussions/Qs but its looking good. 😁
…-to-theme-default-take-2
|
I can't approve the PR since I'm the original author, but anyway I just want to say that I would approve all the work done by @david-szabo97! 🚢 |
…-to-theme-default-take-2
| text-align: left; | ||
|
|
||
| &:focus:not(:disabled) { | ||
| box-shadow: inset 0 0 0 1.5px var(--wp-admin-theme-color), inset 0 0 0 3px #fff; |
There was a problem hiding this comment.
Curious the reasoning behind the 1.5px?
There was a problem hiding this comment.
Nice catch, I think that should be var(--wp-admin-border-width-focus), which is intentionally 1.5px on high res screens, or 2px on low res ones.
Addison-Stavlo
left a comment
There was a problem hiding this comment.
Code looks good & testing around the editor things all work as expected!
Will ✅ - but 1 small nit we should probably update before merging - #28141 (comment)
Addison-Stavlo
left a comment
There was a problem hiding this comment.
Looks good! I noticed one more little nitpick on that style line that I didn't see before.
Approving!
Addison-Stavlo
left a comment
There was a problem hiding this comment.
Oops, I see there are 2 different definitions for that box-shadow now? It looks like we added the var to one and not the other.
Addison-Stavlo
left a comment
There was a problem hiding this comment.
Looks good, thanks!
Description
Fixes #23421
core/edit-siteaction that reverts a template to its original theme-provided file.Technical Notes
"Reverting a template" means:
"Revertable template" indicates a template that
source === 'custom'andoriginal_file_exists(new property).How has this been tested?
wp_templateCPT.wp-admin/edit.php?post_status=trash&post_type=wp_template).Types of changes
New feature (non-breaking change which adds functionality)
Checklist: