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

Site Editor: Do not display 'trashed' navigation menus in Sidebar #55072

Merged
merged 2 commits into from
Oct 5, 2023
Merged

Site Editor: Do not display 'trashed' navigation menus in Sidebar #55072

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
1d9c3f2
Site Editor: Do not display 'trashed' navigation menus in Sidebar
Mamaduka Oct 5, 2023
383c3f1
Extract selector into a hook
Mamaduka Oct 5, 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
9 changes: 2 additions & 7 deletions ...c/components/sidebar-navigation-screen-pattern/template-part-navigation-menu-list-item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,18 @@
/**
* WordPress dependencies
*/
import { useEntityProp } from '@wordpress/core-data';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import SidebarNavigationItem from '../sidebar-navigation-item';
import useNavigationMenuTitle from './use-navigation-menu-title';
import { useLink } from '../routes/link';
import { NAVIGATION_POST_TYPE } from '../../utils/constants';

export default function TemplatePartNavigationMenuListItem( { id } ) {
const [ title ] = useEntityProp(
'postType',
NAVIGATION_POST_TYPE,
'title',
id
);
const title = useNavigationMenuTitle( id );

const linkInfo = useLink( {
postId: id,
Expand Down
10 changes: 2 additions & 8 deletions ...it-site/src/components/sidebar-navigation-screen-pattern/template-part-navigation-menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,15 @@
*/
import { __ } from '@wordpress/i18n';
import { __experimentalHeading as Heading } from '@wordpress/components';
import { useEntityProp } from '@wordpress/core-data';

/**
* Internal dependencies
*/
import NavigationMenuEditor from '../sidebar-navigation-screen-navigation-menu/navigation-menu-editor';
import { NAVIGATION_POST_TYPE } from '../../utils/constants';
import useNavigationMenuTitle from './use-navigation-menu-title';

export default function TemplatePartNavigationMenu( { id } ) {
const [ title ] = useEntityProp(
'postType',
NAVIGATION_POST_TYPE,
'title',
id
);
const title = useNavigationMenuTitle( id );

if ( ! id || title === undefined ) {
return null;
Expand Down
32 changes: 32 additions & 0 deletions ...s/edit-site/src/components/sidebar-navigation-screen-pattern/use-navigation-menu-title.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import { store as coreStore } from '@wordpress/core-data';

/**
* Internal dependencies
*/
import { NAVIGATION_POST_TYPE } from '../../utils/constants';

export default function useNavigationMenuTitle( id ) {
return useSelect(
( select ) => {
if ( ! id ) {
return undefined;
}

const editedRecord = select( coreStore ).getEditedEntityRecord(
'postType',
NAVIGATION_POST_TYPE,
id
);

// Do not display a 'trashed' navigation menu.
return editedRecord.status === 'trash'
? undefined
: editedRecord.title;
},
Copy link
Contributor

Choose a reason for hiding this comment

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

The hook is called useNavigationMenuTitle so I expect it to return the title and not return "empty" even if the record is "trashed". It feels like the logic to show or not something that is "trashed" doesn't really belong to a useNavigationMenuTitle hook. And we may just want to filter some request or something.

Anyway, not that important since it's just internal API but I thought I'd mention.

Copy link
Member Author

Choose a reason for hiding this comment

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

This started as inline useSelect; conditions are more "visible" in that case, and I don't see any issue returning undefined based on record status.

I guess we can return { title, viewable } from the hook to make it more explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest it's not important and I'm fine if leave it but I can clarify my reasoning:

  • useNavigationMenuTitle nothing in the title of the hook suggests anything about the status of the navigation menu. I also think a trashed navigation menu should keep its title.
  • Returning { title, viewable }: yeah that's better but than the name of the hook is not adequate anymore. It feels like this needs two hooks or just leave inline useSelect (what's wrong with that :P).

I know I'm getting into weeds, so don't waste time on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I always appreciate your feedback, @youknowriad!

[ id ]
);
}
Loading