-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Site Editor: Revert custom templates to their original theme-provided files #28141
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
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.
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. |
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.
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 :) |
Ah, I'd never noticed. We can probably address that separately, then. |
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.
Approving the design.
@@ -199,6 +203,11 @@ public function update_item( $request ) { | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
const path = addQueryArgs( entity.baseURL + '/' + key, { | |
...query, | |
context: 'edit', | |
} ); |
I think it should turn out to roughly be something like:
const path = addQueryArgs( `/wp/v2/templates/${ template.id }`, {
is_custom: false,
context: 'edit',
} );
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious the reasoning behind the 1.5px?
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.
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.
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.
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)
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.
Looks good! I noticed one more little nitpick on that style line that I didn't see before.
Approving!
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.
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.
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.
Looks good, thanks!
Description
Fixes #23421
core/edit-site
action 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_template
CPT.wp-admin/edit.php?post_status=trash&post_type=wp_template
).Types of changes
New feature (non-breaking change which adds functionality)
Checklist: