Site Editor: Do not display 'trashed' navigation menus in Sidebar #55072
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] Bug
|
Size Change: +38 B (0%) Total Size: 1.64 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 383c3f1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6415207990
|
aaronrobertshaw
approved these changes
Oct 5, 2023
There was a problem hiding this comment.
Thanks for taking the time to follow up on this @Mamaduka 👍
✅ Trashed navigation do not display in the sidebar
✅ Permanently deleted navigation its do not render anything
✅ Published navigation items continue to display as before
Do you think it makes sense to refactor the duplicated code for retrieving the navigation menu title into a custom hook?
If so, here's a diff you can apply
diff --git a/packages/edit-site/src/components/sidebar-navigation-screen-pattern/template-part-navigation-menu-list-item.js b/packages/edit-site/src/components/sidebar-navigation-screen-pattern/template-part-navigation-menu-list-item.js
index 821b9cb349..22d9d841dc 100644
--- a/packages/edit-site/src/components/sidebar-navigation-screen-pattern/template-part-navigation-menu-list-item.js
+++ b/packages/edit-site/src/components/sidebar-navigation-screen-pattern/template-part-navigation-menu-list-item.js
@@ -1,37 +1,18 @@
/**
* WordPress dependencies
*/
-import { useSelect } from '@wordpress/data';
-import { store as coreStore } 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 = 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;
- },
- [ id ]
- );
+ const title = useNavigationMenuTitle( id );
const linkInfo = useLink( {
postId: id,
diff --git a/packages/edit-site/src/components/sidebar-navigation-screen-pattern/template-part-navigation-menu.js b/packages/edit-site/src/components/sidebar-navigation-screen-pattern/template-part-navigation-menu.js
index 7fe324086d..b7f9b5295f 100644
--- a/packages/edit-site/src/components/sidebar-navigation-screen-pattern/template-part-navigation-menu.js
+++ b/packages/edit-site/src/components/sidebar-navigation-screen-pattern/template-part-navigation-menu.js
@@ -3,35 +3,15 @@
*/
import { __ } from '@wordpress/i18n';
import { __experimentalHeading as Heading } from '@wordpress/components';
-import { useSelect } from '@wordpress/data';
-import { store as coreStore } 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 = 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;
- },
- [ id ]
- );
+ const title = useNavigationMenuTitle( id );
if ( ! id || title === undefined ) {
return null;
diff --git a/packages/edit-site/src/components/sidebar-navigation-screen-pattern/use-navigation-menu-title.js b/packages/edit-site/src/components/sidebar-navigation-screen-pattern/use-navigation-menu-title.js
new file mode 100644
index 0000000000..4585c98ce3
--- /dev/null
+++ b/packages/edit-site/src/components/sidebar-navigation-screen-pattern/use-navigation-menu-title.js
@@ -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;
+ },
+ [ id ]
+ );
+}
One last question, should the "trash" and "delete permanently" links in the `wp-admin/edit.php?post_type=wp_navigation" screen take you to the site editor? The only way I could delete the different navigation was to use the bulk actions.
Screen.Recording.2023-10-05.at.3.31.08.pm.mp4
Co-authored-by: Aaron Robertshaw <[email protected]>
|
Thanks, @aaronrobertshaw! Extracted logic into a hook.
I've also noticed that. I think those actions shouldn't open the Site Editor. But I'm guessing they're both using Here's the core sync PR for that change - WordPress/wordpress-develop#4710. Maybe @ramonjd or @getdave have ideas on how to fix it. |
Mamaduka
added
the
Backport to WP 6.7 Beta/RC
|
How do we handle this for template parts or is the same bug manifested there? |
|
Template parts don't allow accessing the default post list view. Try: |
mikachan
pushed a commit
that referenced
this pull request
Oct 5, 2023
…5072) * Site Editor: Do not display 'trashed' navigation menus in Sidebar * Extract selector into a hook Co-authored-by: Aaron Robertshaw <[email protected]> --------- Co-authored-by: Aaron Robertshaw <[email protected]>
|
I just cherry-picked this PR to the 6.4-beta3 branch to get it included in the next release: d0630d0 |
mikachan
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
reviewed
Oct 5, 2023
| return editedRecord.status === 'trash' | ||
| ? undefined | ||
| : editedRecord.title; | ||
| }, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
To be honest it's not important and I'm fine if leave it but I can clarify my reasoning:
useNavigationMenuTitlenothing 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.
There was a problem hiding this comment.
I always appreciate your feedback, @youknowriad!
karmatosed
pushed a commit
that referenced
this pull request
Oct 9, 2023
* Add clearer labels and context to BlockPatternsSyncFilter (#54838) * Add help text & clearer labeling * Theme & Plugins * Font Library: use snake_case instead of camelCase on fontFamilies endpoint param (#54977) * use snake_case instead of camelCase on endpoint param * updating test * Fix output of Navigation block classnames in the editor. (#54992) * Block Editor: Avoid double-wrapping selectors when transforming the styles (#54981) * Block Editor: Avoid double-wrapping selectors when transforming the styles * Include space in the check * Don't display the navigation section in template parts details when a menu is missing (#54993) * Scripts: Properly use CommonJS for default Playwright config (#54988) * Fix path to `globalSetup` in default Playwright config Oversight from #54856 * `module.exports` * Fix default export usage * Add template replace flow to template inspector (#54609) * Add a modal to allow template switching * fetch template info * Allow switching to different patterns * Allow switching to different patterns * Add columns * move availble templates to the actions * filter for the correct templates * create the right data structure in the use select * move to a hook * inject theme attribute into pattern again * put the overlay over the top of the dropdown * fix the pattern to templates hook * set the template on click * Also set the blocks * remove calls to set template with the current template, since setting blocks correctly updates the content in the editor * serialize blocks so that we have correctly processed template parts * remove duplicated code * Remove unnecessary mapping * refactor * memoize the patterns * combine the useSelect * Update packages/edit-site/src/components/sidebar-edit-mode/page-panels/hooks.js Co-authored-by: Andrei Draganescu <[email protected]> * Fix ESLint error * Only show the button is there is more than 1 pattern * Copy update * Move the hook to a subdir * check that there are patterns * move the check * remove useCallback * change condition to show the button * change condition * move to use editEntityRecord * combine filters * add comments * Update packages/edit-site/src/components/sidebar-edit-mode/template-panel/replace-template-button.js Co-authored-by: Andrei Draganescu <[email protected]> --------- Co-authored-by: Andrei Draganescu <[email protected]> Co-authored-by: Andrei Draganescu <[email protected]> Co-authored-by: George Mamadashvili <[email protected]> * List View: Fix performance issue when selecting all blocks (#54900) * List View: Fix performance issue when selecting all blocks within the editor canvas in long posts * Add a comment, rename const * Move block focus to be performed only once at the root of the list view, instead of within each block * Fix left and right aligmnent in children of Post Template (#54997) * Fix left and right aligmnent in children of Post Template * Add align center styles * Fix image placeholder disappearing * Site Editor: Avoid stale navigation block values when parsing entity record (#54996) * Removed unwanted space from the string (#54654) * Update styles.js Removed unwanted space with translation * Update deleted-navigation-warning.js Unwanted space at the end of the string shows translation warning. * Update inspector-controls.js Unwanted space at the end of the string shows translation warning * Fix Deleted Navigation Menu warning string (#55033) * [Inserter]: Fix reset of registered media categories (#55012) * [Inserter]: Fix reset of registered media categories * convert `useInserterMediaCategories` to selector and make private * Try fixing the flaky 'Toolbar roving tabindex' e2e test (#54785) * Try fixing the flaky 'Toolbar roving tabindex' e2e test * Add a link in the comment * Fallback to Twitter provider when embedding X URLs (#54876) * Fallback to Twitter provider when embedding X URLs * Avoid processing empty urls when trying a different provider * Update `react-native-editor` changelog # Conflicts: # packages/react-native-editor/CHANGELOG.md * Based on the efforts in #51761, remove caps case from Template Part and prefer sentence case. As all instances of this string are stand alone, it's okay to have Template capitalized as it's the start of a sentence. (#54709) * Update pattern import menu item (#54782) * Update pattern import menuitem * Revert label * Image Block: Fix browser console error when clicking "Expand on Click" (#54938) * Patterns: Remove category description in inserter panel? (#54894) * Media & Text: Fix React warning (#55038) * Block Style: Display default style labels correctly in the block sidebar (#55008) * Site Editor: Do not display 'trashed' navigation menus in Sidebar (#55072) * Site Editor: Do not display 'trashed' navigation menus in Sidebar * Extract selector into a hook Co-authored-by: Aaron Robertshaw <[email protected]> --------- Co-authored-by: Aaron Robertshaw <[email protected]> * Image: Fix Lightbox display bug in Classic Themes. (#54837) * If current theme is not a block theme add a default value for $background_color and $close_button_color. * Set lightbox buttons' background & border to none on hover & focus * Change logic to support lightbox in classic themes * Update logic to avoid unnecessary calls * Add style fixes * Remove unnecessary code * Fix close button color --------- Co-authored-by: Mario Santos <[email protected]> Co-authored-by: Ricardo Artemio Morales <[email protected]> * Latest Posts: add screen reader title text to Read more links and use alternative to excerpt_more filter (#55029) * In the editor: adds a screen reader text span with the post title in the i18n interpolator In the frontend: removes excerpt_more filter so we don't override themes and also replaces the default ellipsis with an accessible read more link * Removing "of" preposition in favour of a semi-colon. "Read more" is already translated so using a specifier to add it to the string * Fix Image block lightbox missing alt attribute and improve accessibility (#55010) * Move lightbox open button after the image. * Fix getting the lightbox image alt attribute. * Improve docblocks. * Do not render empty role attribute. * Remove unnecessary aria-hidden attribute. * Set aria-modal attribute dynamically. * More meaningful and simpler modal dialog aria-label. * Increase Close button target size. * Add enlarged image base64 encoded placeholder. * Better check for alt attribute as a string. * Update changelog. * Move changelog entry to the block library changelog. * Set lightbox dialog aria-label dynamically. * Hide background scrim container from assistive technology. * Remove obsolete code --------- Co-authored-by: Ricardo Artemio Morales <[email protected]> # Conflicts: # packages/block-library/CHANGELOG.md * Patterns: Add category selector to pattern creation modal (#55024) --------- Co-authored-by: Kai Hao <[email protected]> * Iframe: Fix positioning when dragging over an iframe (#55150) * Site Editor: Fix template part area listing when a template has no edits (#55115) * Alternative: Fix template part area listing when a template has no edits * Fix typos --------- Co-authored-by: Rich Tabor <[email protected]> Co-authored-by: Matias Benedetto <[email protected]> Co-authored-by: tellthemachines <[email protected]> Co-authored-by: George Mamadashvili <[email protected]> Co-authored-by: Pascal Birchler <[email protected]> Co-authored-by: Ben Dwyer <[email protected]> Co-authored-by: Andrei Draganescu <[email protected]> Co-authored-by: Andrei Draganescu <[email protected]> Co-authored-by: Andrew Serong <[email protected]> Co-authored-by: mujuonly <[email protected]> Co-authored-by: Dave Smith <[email protected]> Co-authored-by: Nik Tsekouras <[email protected]> Co-authored-by: Carlos Garcia <[email protected]> Co-authored-by: Ramon <[email protected]> Co-authored-by: James Koster <[email protected]> Co-authored-by: Aki Hamano <[email protected]> Co-authored-by: Aaron Robertshaw <[email protected]> Co-authored-by: Michal <[email protected]> Co-authored-by: Mario Santos <[email protected]> Co-authored-by: Ricardo Artemio Morales <[email protected]> Co-authored-by: Andrea Fercia <[email protected]> Co-authored-by: Glen Davies <[email protected]> Co-authored-by: Kai Hao <[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?
This is a follow-up to #54996 (comment).
PR updates selectors for template parts navigation menu display components to avoid rendering trashed menu items.
Why?
The Navigation block considers entities with this status as deleted. This fixes inconsistency.
Testing Instructions
wp-admin/edit.php?post_type=wp_navigationscreen.Testing Instructions for Keyboard
Same.
Screenshot