Navigation Sidebar: Change the logic about which navigation gets selected for the sidebar #48689
Conversation
…cted for the sidebar
|
Size Change: -37 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in c5eb33d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4323167073
|
| } | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| export default function SidebarNavigationScreenNavigationMenus() { |
There was a problem hiding this comment.
I was forced to merge NavigationInspector into this component because the "BlockEditorProvider" now needs to wrap the SidebarNavigationScreen component (because the appender moved to the "actions" section there)
| ); | ||
|
|
||
| // This is copied from the edit component of the Navigation block. | ||
| const orderedNavigationMenus = useMemo( |
There was a problem hiding this comment.
I was wondering if we could avoid this entirely, by passing orderBy something to the query? Don't we have that already? cc @scruffian
There was a problem hiding this comment.
The status publish filter is already on the query so at least that part can already be removed.
| > | ||
| <div className="edit-site-sidebar-navigation-screen-navigation-menus__content"> | ||
| <NavigationMenuContent | ||
| innerBlocks={ publishedInnerBlocks } |
There was a problem hiding this comment.
I think now that we're actually rendering a hidden BlockList we might not need to wait for "innerBlocks" loading and there's also some action calls that could be removed from NavigationMenuContent. I kept them for now but I'll see later if I can get rid of them and simplify further.
|
The description, and relocation of the [+] help a lot. For the latter, should we change the tooltip? It currently reads "Add block". I appreciate everything inside the menu is technically a block, but I don't know if that's how people think about things in this context. Perhaps "Add menu item"? I think it would be nice to mark the main/primary template in the UI:
This would eliminate any guesswork when you want to use this menu elsewhere. |
|
ok I made a small mistake on my last commit. Trying to fix quickly, otherwise I'll revert |
|
Just took your latest for a spin. Looking very close. Using a Page List I can navigate the blocks correctly. One small missing piece, perhaps something to look at in #47748, is that when I drag and drop items in the page list in the dark material on the left, it's not working. It would be fine if this invoked the modal, "do you want to edit?", or (IMO preferrably) skipped that modal entirely and just went ahead and saved a menu as soon as you made a drag and drop action. |
|
Here are two issues I noticed that IMO should be fixed independently from this PR:
I'll try an approach to solve the first one in a separate PR (probably next week though, as I'm going AFK tomorrow). |
There was a problem hiding this comment.
This PR seems ready and works well. Thank you for these changes 👍 The only issue I'm facing is the inability to insert some items like the custom link.
Should we also have the 6.1 behavior where for simple menus we insert the custom link right away when clicking the insert button? Custom link insert UI allows things like creating new pages from the menu. This can be done in a different PR given that the current behavior is already not that one. And if we change that behavior we would also need to change it inside the off-canvas editor.
| ); | ||
|
|
||
| // This is copied from the edit component of the Navigation block. | ||
| const orderedNavigationMenus = useMemo( |
There was a problem hiding this comment.
The status publish filter is already on the query so at least that part can already be removed.
| const NAVIGATION_MENUS_QUERY = { per_page: -1, status: 'publish' }; | ||
| const settings = { | ||
| allowedBlockTypes: [ | ||
| 'core/navigation-link', |
There was a problem hiding this comment.
The navigation link is an allowed block but some blocks like "Custom Link" and "Post Link" don't appear on the inserter and is impossible to add them:
I'm not sure yet why they don't appear 🤔
There was a problem hiding this comment.
Is this also a problem in trunk?
There was a problem hiding this comment.
I've explained the reasons for this here #48689 (comment)
I'm not sure why it doesn't happen in trunk but I'll explore a fix for this
There was a problem hiding this comment.
Ok, I've fixed the issue. There's one small thing remaining though. You can see when loading the page that the "navigation menu" appears "empty" for a second. I didn't find the right check to add to maintain the isLoading state properly until all the inner blocks are loaded. Maybe @scruffian has an idea on what we can check here.
There was a problem hiding this comment.
I found a small hack to load for the navigation block to load its inner blocks but I still see like a couple milliseconds where the navigation menu shows "an empty state". It's not perfect but maybe it's good enough to start.
youknowriad
added
[Type] Enhancement
|
Should the description mention menu editing first, since it is the 'Navigation' panel?
Not a strong opinion. |
| const firstNavigationMenu = orderedNavigationMenus?.[ 0 ]?.id; | ||
| const blocks = useMemo( () => { | ||
| return [ | ||
| createBlock( 'core/navigation', { ref: firstNavigationMenu } ), |
…cted for the sidebar (#48689) * Navigation Sidebar: Change the logic about which navigation gets selected for the sidebar * Move the navigation sidebar appender into the actions area of the sidebar * Further simplification * Fixed allowed blocks and update appender label * Fix inserter issue * Delay the rendering of the navigation menus * Remove useless code * update comment --------- Co-authored-by: Riad Benguella <benguella@gmail.com>
|
Cherry-picked this PR to the wp/6.2 branch. |
ntsekouras
removed
the
Backport to WP 6.7 Beta/RC
|
Hey @scruffian, I'm coming back to this one way late, but I noticed that this branch added the following loading state:
We don't really use a loading state like this anywhere else. Can you replace it with a spinner? And ideally a spinner that is delayed by perhaps .2ms so that it only appears if there's actually any loading to do. |
If so we can close #48682 🚀 |
Closes #48690
Closes #48665
What?
This PR updates the navigation menus in the site editor with the following changes:
Testing Instructions
1- Open the site editor
2- Click "Navigation" on the sidebar
3- Play with the changes.