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

Restore: Move to trash button in Document settings #65087

Merged
merged 4 commits into from
Sep 6, 2024
Merged

Restore: Move to trash button in Document settings #65087

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
4d169f6
avoid calling usePostActions on trash eligibility check (+2 squashed …
jorgefilipecosta Sep 5, 2024
e0769fc
don't use the action mechanism make the modals match
jorgefilipecosta Sep 5, 2024
3dac80e
update checks
jorgefilipecosta Sep 5, 2024
64990ed
update check
jorgefilipecosta Sep 6, 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
7 changes: 5 additions & 2 deletions packages/editor/src/components/post-trash/check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { store as coreStore } from '@wordpress/core-data';
* Internal dependencies
*/
import { store as editorStore } from '../../store';
import { GLOBAL_POST_TYPES } from '../../store/constants';

/**
* Wrapper component that renders its children only if the post can trashed.
Expand All @@ -34,10 +35,12 @@ export default function PostTrashCheck( { children } ) {
: false;

return {
canTrashPost: ( ! isNew || postId ) && canUserDelete,
canTrashPost:
( ! isNew || postId ) &&
canUserDelete &&
! GLOBAL_POST_TYPES.includes( postType ),
};
}, [] );

if ( ! canTrashPost ) {
return null;
}
Expand Down
18 changes: 11 additions & 7 deletions packages/editor/src/components/post-trash/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we refactor this component? What was wrong with the previous implementation?

Copy link
Member Author

@jorgefilipecosta jorgefilipecosta Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We should not have two different modals to trash, we should rely on the one the actions already offer.
E.g: less strings to translate. The UI when pressing trash on actions and button also becomes the same. It is unexpected to get one UI if I press it in a given way and different UI if I press it in other way.

Copy link
Member Author

@jorgefilipecosta jorgefilipecosta Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This update also handles cases where a plugin removes an action, such as the move to trash action. With these changes, we also don't render this button if the action is removed.

Copy link
Member

@Mamaduka Mamaduka Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I also don't see a reason for this change. Original components are well-tested, and we're not introducing any new code, so "duplication" is justified.

If the modals don't match, we can just update the confirmation modal in the PostTrash component.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Mamaduka as referred above we have API's to remove actions unregisterEntityAction, so I expect that if I unregister the move to trash action this button goes away. It becomes inconsistent that the actions go away on the actions menu but not on this button. If we need to retrieve the action for this check why not just use the modal the action provides?

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 that reuse the actions would be good, but my opinion is that the way we're reusing them now (with an adhoc component within the editor package and not exported from the dataviews package) is not good.

I think Actions are a dataviews API and should ideally not be used in other things that are not dataviews components.

Copy link
Member

@Mamaduka Mamaduka Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jorgefilipecosta, the unregisterEntityAction controls one part of the UI - entity actions. I don't expect them to affect other parts.

E.g., If someone removes the "Rename" action, should we also hide the Post Title field in the canvas?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair points, I followed the suggestions and updated to keep the previous logic, but changed the modal to the be similar to the actions one.

import { __, sprintf } from '@wordpress/i18n';
import {
Button,
__experimentalConfirmDialog as ConfirmDialog,
Expand All @@ -13,19 +13,21 @@ import { useState } from '@wordpress/element';
* Internal dependencies
*/
import { store as editorStore } from '../../store';
import PostTrashCheck from './check';

/**
* Displays the Post Trash Button and Confirm Dialog in the Editor.
*
* @return {JSX.Element|null} The rendered PostTrash component.
*/
export default function PostTrash() {
const { isNew, isDeleting, postId } = useSelect( ( select ) => {
const { isNew, isDeleting, postId, title } = useSelect( ( select ) => {
const store = select( editorStore );
return {
isNew: store.isEditedPostNew(),
isDeleting: store.isDeletingPost(),
postId: store.getCurrentPostId(),
title: store.getCurrentPostAttribute( 'title' ),
};
}, [] );
const { trashPost } = useDispatch( editorStore );
Expand All @@ -41,7 +43,7 @@ export default function PostTrash() {
};

return (
<>
<PostTrashCheck>
<Button
__next40pxDefaultSize
className="editor-post-trash"
Expand All @@ -60,12 +62,14 @@ export default function PostTrash() {
onConfirm={ handleConfirm }
onCancel={ () => setShowConfirmDialog( false ) }
confirmButtonText={ __( 'Move to trash' ) }
size="medium"
size="small"
>
{ __(
'Are you sure you want to move this post to the trash?'
{ sprintf(
// translators: %s: The item's title.
__( 'Are you sure you want to move "%s" to the trash?' ),
title
) }
</ConfirmDialog>
</>
</PostTrashCheck>
);
}
2 changes: 2 additions & 0 deletions packages/editor/src/components/sidebar/post-summary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import PostsPerPage from '../posts-per-page';
import SiteDiscussion from '../site-discussion';
import { store as editorStore } from '../../store';
import { PrivatePostLastRevision } from '../post-last-revision';
import PostTrash from '../post-trash';

/**
* Module Constants
Expand Down Expand Up @@ -87,6 +88,7 @@ export default function PostSummary( { onActionPerformed } ) {
<SiteDiscussion />
<PostFormatPanel />
</VStack>
<PostTrash />
{ fills }
</VStack>
) }
Expand Down
Loading