Site Editor: Fix template parts 'Reset' action #62951
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
added
[Type] Regression
|
Size Change: -188 B (-0.01%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
Mamaduka
commented
Jun 28, 2024
| template.type === TEMPLATE_PART_POST_TYPE | ||
| ? isTemplatePartRevertable | ||
| : isTemplateRevertable; | ||
| if ( ! isRevertable( template ) ) { | ||
| registry | ||
| .dispatch( noticesStore ) | ||
| .createErrorNotice( __( 'This template is not revertable.' ), { |
There was a problem hiding this comment.
The message needs to reflect the type, but the fix needs to be backported into RC, and we're currently in the "hard string-freeze" phase.
I'll handle string updates separately.
Mamaduka
commented
Jun 28, 2024
Comment on lines
-907
to
-829
| if ( items[ 0 ].type === TEMPLATE_PART_POST_TYPE ) { | ||
| await removeTemplates( items ); | ||
| } else { |
There was a problem hiding this comment.
@jorgefilipecosta, this special condition was introduced in #61612, but I couldn't find any details on why it was necessary.
Calling removeTemplates when resetting a template part from the Site Editor causes a missing entity error.
There was a problem hiding this comment.
Hi @Mamaduka,
In #61612 I implemented things this way to replicate the logic that already existed before for template part reset.
Previously when a template part reset happened the template part post of the user was deleted it was always the case so I kept the behavior.
With this change instead of deleting we keep the user template part and change it to be equal to what the template set. This may cause issues during updates etc.
Previously @youknowriad even said that maybe even for templates during a reset we should just delete the user template too. I'm not sure if going in the direction of keeping the user entity also for template parts is a good idea.
There was a problem hiding this comment.
Update: In my tests, it seems the template part is still deleted as expected so things look good. I'm checking how it happens given that revertTemplate at least previously did not apply a delete 🤔
There was a problem hiding this comment.
Checking the code from wp/6.5 branch. It looks like that the editor sidebar was using the revertTemplate while "DataViews" was using the removeTemplates action.
The problem with using removeTemplates while viewing the same entity in canvas is that it also deletes the entity from the store and requires page refresh to load it.
Here's what happens when I restore old behavior:
CleanShot.2024-07-03.at.13.54.24.mp4
There was a problem hiding this comment.
On revert templates when we pass source: 'theme', to edit an entity it is in fact an instruction to delete the entity from the database. I will add a comment there to make the code easier to read in the future, I was not aware of that.
But yah we should apply this simplification as we can use the use behavior on both.
There was a problem hiding this comment.
Thank you, @jorgefilipecosta!
Mamaduka
commented
Jun 28, 2024
Comment on lines
62
to
67
| const canDeleteOrReset = ( item ) => { | ||
| const isTemplatePart = item.type === TEMPLATE_PART_POST_TYPE; | ||
| const isUserPattern = item.type === PATTERN_TYPES.user; | ||
| return isUserPattern || ( isTemplatePart && item.isCustom ); | ||
| }; |
There was a problem hiding this comment.
I've removed this check.
Why?
The "Reset" action is only available for the wp_template and wp_template_part entities.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Mamaduka
force-pushed
the
fix/template-parts-reset-action
branch
from
ed6b88f to
55e2c7f
Compare
richtabor
approved these changes
Jul 1, 2024
Mamaduka
force-pushed
the
fix/template-parts-reset-action
branch
2 times, most recently
from
3d1e5c3 to
9a30180
Compare
|
@youknowriad, I think your #63017 PR could supersede this unless there's another 6.6 RC; in that case, this probably should go in first. Additionally, some of the notes from this PR could also apply there. cc @ellatrix |
Mamaduka
added
the
Backport to WP 6.7 Beta/RC
|
Looks solid to me, this is good to include. Would be good to have an extra check from @jorgefilipecosta |
youknowriad
reviewed
Jul 3, 2024
| * @param {Object} templatePart The template part entity to check. | ||
| * @return {boolean} Whether the template part is revertable. | ||
| */ | ||
| export default function isTemplatePartRevertable( templatePart ) { |
There was a problem hiding this comment.
There's already a function called isTemplateRevertable which I believe is the exact same function (we can reuse it by naming the argument templateOrTemplatePart
|
I'm happy to update my PR after this one lands. I don't think I changed the logic much in my PR |
Mamaduka
force-pushed
the
fix/template-parts-reset-action
branch
from
ab9e7b1 to
1a29800
Compare
ellatrix
reviewed
Jul 3, 2024
jorgefilipecosta
approved these changes
Jul 3, 2024
|
There was a conflict while trying to cherry-pick the commit to the wp/6.6 branch. Please resolve the conflict manually and create a PR to the wp/6.6 branch. PRs to wp/6.6 are similar to PRs to trunk, but you should base your PR on the wp/6.6 branch instead of trunk. |
Mamaduka
added a commit
that referenced
this pull request
Jul 3, 2024
Co-authored-by: Mamaduka <[email protected]> Co-authored-by: richtabor <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: jorgefilipecosta <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: t-hamano <[email protected]>
Mamaduka
removed
the
Backport to WP 6.7 Beta/RC
Mamaduka
added a commit
that referenced
this pull request
Jul 5, 2024
Co-authored-by: Mamaduka <[email protected]> Co-authored-by: richtabor <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: jorgefilipecosta <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: t-hamano <[email protected]>
Mamaduka
added a commit
that referenced
this pull request
Jul 5, 2024
Co-authored-by: richtabor <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: jorgefilipecosta <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: t-hamano <[email protected]>
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
Co-authored-by: Mamaduka <[email protected]> Co-authored-by: richtabor <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: jorgefilipecosta <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: t-hamano <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #62604.
PR fixes the "is eligible" condition for template parts and makes the "Reset" action work properly.
Why?
The action was no longer available for template part entities.
Testing Instructions
Testing Instructions for Keyboard
Same.
Screenshots or screencast
Editor sidebar
DataViews
