Skip to content
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.

Sign up for GitHub

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: Fix template parts 'Reset' action #62951

Merged
merged 4 commits into from
Jul 3, 2024
Merged

Site Editor: Fix template parts 'Reset' action #62951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c9d2d64
Site Editor: Fix template parts 'Reset' action
Mamaduka Jun 28, 2024
2018be8
Fix the 'Reset' action in dataviews
Mamaduka Jun 28, 2024
1a29800
Reuse the check
Mamaduka Jul 3, 2024
413cba3
Cleanup
Mamaduka Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 27 additions & 53 deletions packages/editor/src/components/post-actions/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,6 @@ function isTemplateRemovable( template ) {
! template?.has_theme_file
);
}
const canDeleteOrReset = ( item ) => {
const isTemplatePart = item.type === TEMPLATE_PART_POST_TYPE;
const isUserPattern = item.type === PATTERN_TYPES.user;
return (
isUserPattern ||
( isTemplatePart && item.source === TEMPLATE_ORIGINS.custom )
);
};

function getItemTitle( item ) {
if ( typeof item.title === 'string' ) {
Expand Down Expand Up @@ -795,68 +787,50 @@ const useDuplicatePostAction = ( postType ) => {
);
};

const isTemplatePartRevertable = ( item ) => {
if ( ! item ) {
return false;
}
const hasThemeFile = item?.has_theme_file;
return canDeleteOrReset( item ) && hasThemeFile;
};

const resetTemplateAction = {
id: 'reset-template',
label: __( 'Reset' ),
isEligible: ( item ) => {
return item.type === TEMPLATE_PART_POST_TYPE
? isTemplatePartRevertable( item )
: isTemplateRevertable( item );
return isTemplateRevertable( item );
},
icon: backup,
supportsBulk: true,
hideModalHeader: true,
RenderModal: ( { items, closeModal, onActionPerformed } ) => {
const [ isBusy, setIsBusy ] = useState( false );
const { revertTemplate, removeTemplates } = unlock(
useDispatch( editorStore )
);
const { revertTemplate } = unlock( useDispatch( editorStore ) );
const { saveEditedEntityRecord } = useDispatch( coreStore );
const { createSuccessNotice, createErrorNotice } =
useDispatch( noticesStore );
const onConfirm = async () => {
try {
if ( items[ 0 ].type === TEMPLATE_PART_POST_TYPE ) {
await removeTemplates( items );
} else {
Comment on lines -827 to -829
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jorgefilipecosta jorgefilipecosta Jul 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @jorgefilipecosta!

for ( const template of items ) {
if ( template.type === TEMPLATE_POST_TYPE ) {
await revertTemplate( template, {
allowUndo: false,
} );
await saveEditedEntityRecord(
'postType',
template.type,
template.id
);
}
}
createSuccessNotice(
items.length > 1
? sprintf(
/* translators: The number of items. */
__( '%s items reset.' ),
items.length
)
: sprintf(
/* translators: The template/part's name. */
__( '"%s" reset.' ),
decodeEntities( getItemTitle( items[ 0 ] ) )
),
{
type: 'snackbar',
id: 'revert-template-action',
}
for ( const template of items ) {
await revertTemplate( template, {
allowUndo: false,
} );
await saveEditedEntityRecord(
'postType',
template.type,
template.id
);
}
createSuccessNotice(
items.length > 1
? sprintf(
/* translators: The number of items. */
__( '%s items reset.' ),
items.length
)
: sprintf(
/* translators: The template/part's name. */
__( '"%s" reset.' ),
decodeEntities( getItemTitle( items[ 0 ] ) )
),
{
type: 'snackbar',
id: 'revert-template-action',
}
);
} catch ( error ) {
let fallbackErrorMessage;
if ( items[ 0 ].type === TEMPLATE_POST_TYPE ) {
Expand Down
16 changes: 8 additions & 8 deletions packages/editor/src/store/utils/is-template-revertable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,18 @@ import { TEMPLATE_ORIGINS } from '../constants';
// Copy of the function from packages/edit-site/src/utils/is-template-revertable.js

/**
* Check if a template is revertable to its original theme-provided template file.
* Check if a template or template part is revertable to its original theme-provided file.
*
* @param {Object} template The template entity to check.
* @return {boolean} Whether the template is revertable.
* @param {Object} templateOrTemplatePart The entity to check.
* @return {boolean} Whether the entity is revertable.
*/
export default function isTemplateRevertable( template ) {
if ( ! template ) {
export default function isTemplateRevertable( templateOrTemplatePart ) {
if ( ! templateOrTemplatePart ) {
return false;
}
/* eslint-disable camelcase */

return (
template?.source === TEMPLATE_ORIGINS.custom && template?.has_theme_file
templateOrTemplatePart.source === TEMPLATE_ORIGINS.custom &&
templateOrTemplatePart.has_theme_file
);
/* eslint-enable camelcase */
}
Loading