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

Jump to bottom

Show confirmation dialog when moving a post to the trash #50219

Merged
merged 2 commits into from
Oct 4, 2023
Merged

Show confirmation dialog when moving a post to the trash #50219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d271ec2
Show confirmation dialog when moving a post to the trash
mtias May 1, 2023
8360ac7
Update change detection e2e test
Mamaduka Oct 4, 2023
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
1 change: 1 addition & 0 deletions packages/e2e-tests/specs/editor/various/change-detection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,7 @@ describe( 'Change detection', () => {
// Trash post.
await openDocumentSettingsSidebar();
await page.click( '.editor-post-trash.components-button' );
await page.click( '.components-confirm-dialog .is-primary' );

await Promise.all( [
// Wait for "Saved" to confirm save complete.
Expand Down
45 changes: 34 additions & 11 deletions packages/editor/src/components/post-trash/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,12 @@
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { Button } from '@wordpress/components';
import {
Button,
__experimentalConfirmDialog as ConfirmDialog,
} from '@wordpress/components';
import { useSelect, useDispatch } from '@wordpress/data';
import { useState } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -20,21 +24,40 @@ export default function PostTrash() {
};
}, [] );
const { trashPost } = useDispatch( editorStore );
const [ showConfirmDialog, setShowConfirmDialog ] = useState( false );

if ( isNew || ! postId ) {
return null;
}

const handleConfirm = () => {
setShowConfirmDialog( false );
trashPost();
};

return (
<Button
className="editor-post-trash"
isDestructive
variant="secondary"
isBusy={ isDeleting }
aria-disabled={ isDeleting }
onClick={ isDeleting ? undefined : () => trashPost() }
>
{ __( 'Move to trash' ) }
</Button>
<>
<Button
className="editor-post-trash"
isDestructive
variant="secondary"
isBusy={ isDeleting }
aria-disabled={ isDeleting }
onClick={
isDeleting ? undefined : () => setShowConfirmDialog( true )
}
>
{ __( 'Move to trash' ) }
</Button>
<ConfirmDialog
isOpen={ showConfirmDialog }
onConfirm={ handleConfirm }
onCancel={ () => setShowConfirmDialog( false ) }
>
{ __(
'Are you sure you want to move this post to the trash?'
Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably avoid the term "post" or contextualize with whatever post type is the current context.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Current context would be best here.

Copy link
Member

Choose a reason for hiding this comment

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

Should we consider this as a blocker? cc @jasmussen

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree current context would be best, but since we are currently shipping in trunk without the warning, it seems an improvement to add one as a first step, but we can just rephrase to this:

Are you sure you want to move this to the trash?

Copy link
Member

Choose a reason for hiding this comment

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

I just checked, and "switch to draft" also uses the "post" term in the confirmation message. I think we can merge this as it is and contextualize both in follow-up PR.

) }
</ConfirmDialog>
</>
);
}
Loading