Fix: Make navigation page list load its items on navigation sidebar. #47853
Conversation
jorgefilipecosta
added
the
[Type] Bug
|
Size Change: +77 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 8c2f80d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4284293063
|
jorgefilipecosta
force-pushed
the
fix/make-navigation-page-list-load-its-items-on-sidebar
branch
6 times, most recently
from
f700b58
to
55a71ae
Compare
| <> | ||
| <div className="edit-site-navigation-inspector__placeholder is-child" /> | ||
| <div className="edit-site-navigation-inspector__placeholder is-child" /> | ||
| <div className="edit-site-navigation-inspector__placeholder is-child" /> | ||
| </> | ||
| ) } | ||
| { hasInnerBlocks && ! isLoading && ( | ||
| <BlockEditorProvider |
There was a problem hiding this comment.
If we're removing the provider from here, where do we get the "blocks" from?
There was a problem hiding this comment.
Hi @youknowriad, we are now using the main editor instance, and we retrieve the navigation block from there. If we call wp.data.select('core/block-editor).geyBlocks(); we have a tree, and the navigation blocks are there. We are just using it.
We kept the previous logic where the menu shown is associated with the first navigation block or the selected navigation block.
There was a problem hiding this comment.
That doesn't make a lot of sense to me, the main editor is rendered in the "frame" and its content might change. For instance I can see a website where the home page doesn't contain any menu but you need to get into an internal page first.
In other words, I still think the inspector is its own block editor instance and we should be able to control what we load in it and not depend on what's shown elsewhere (frame)
There was a problem hiding this comment.
That doesn't make a lot of sense to me, the main editor is rendered in the "frame" and its content might change. For instance I can see a website where the home page doesn't contain any menu but you need to get into an internal page first.
If the menu is not on the current frame, how do we know the menu to show? Multiple templates may even reference different navigation menus. If the menu is not in the frame to me, it seems better not to show the navigation option at all, given that we don't have navigation.
The current system using a different block editor does not seem reliable:
- Page lists don't load at all because the edit function is not executed.
- There are many crashes when using two block editor providers where both reference the menu entity. E.g: try to add some menu items on the sidebar, remove, etc; soon the editor crashes.
In the previous system, the one we have on the trunk and that comes from the navigation inspector sidebar days, if there was no menu referenced in the frame just showed the first navigation menu even if no template references it..
I guess we can merge this PR using a single block editor and we use a single editor every time is possible. If we really want to show the navigation editor even if navigation is not in the frame, I can add a follow-up that in that case, loads the first menu as before in a new block editor provider. Given that in that case there are not multiple block-editor providers referencing the same entity I guess we will not have crashes. And doing this we keep the same functionality as before. How does that sound?
There was a problem hiding this comment.
For clarity, In trunk right now, when the "navigation menus" is shown in the sidebar, we're always showing the home page.
If the menu is not on the current frame, how do we know the menu to show?
I think that's the main question that we need to solve here. IMO, the sidebar is higher-level than the frame in other words, the frame should never dictate what's shown in the sidebar, it's the other way around.
From that perspective, Any of the following approaches are good to me:
- Show the first menu with a "select" to switch which menu to show
- Show the first menu with the assumption that most sites only have a single menu (so don't bother with the "select")
- Have a way (could be a "guideline" or "convention") to know which menu is the "main menu" of the site and always show that one.
There was a problem hiding this comment.
Hi @youknowriad, I updated the code now we keep the behavior of loading the first navigation menu in a new block editor if it is not in canvas. When it is in canvas we don't load a new block editor which solves many crashes.
When the navigation menu is not in canvas we render a hidden block editor so edit functions are executed and things like the page list load correctly.
With the current state, we don't lose any functionality when compared to trunk.
There was a problem hiding this comment.
. When it is in canvas we don't load a new block editor which solves many crashes.
@jorgefilipecosta I think we should always have a separate block editor in that sense, we shouldn't be assuming that a canvas exists in the first place.
solves many crashes.
I'd like to understand what crashes it solves, because for me this is an indication that we have bugs elsewhere and we're just hacking our way around it.
There was a problem hiding this comment.
I'm not being able to reproduce the crashes right now. Previously they were very frequent and easy to replicate something probably changed meanwhile. The crashes were from react saying we have recursive calls to setState.
I will pay attention and see if they come back.
I kept exactly the same logic as before, and reverted back to my first commit where I'm only adding a hidden block editor so edit functions execute and page lists load.
I still think if the navigation block is loaded on the canvas it would be better to not load a new block editor and hidden block list it would be more performant to have a single block editor instead of two. But in order to keep this PR simple and focus on the page list I guess we can go with the hidden block list for now.
| history.push( { | ||
| postType: 'page', | ||
| postId: attributes.id, | ||
| path: '/navigation/single', |
There was a problem hiding this comment.
I'm working on a PR to remove this, so we might want to check which one lands first :)
jorgefilipecosta
force-pushed
the
fix/make-navigation-page-list-load-its-items-on-sidebar
branch
2 times, most recently
from
4b61df1
to
30afe97
Compare
With the changes on #47853, we are not using a specific block editor for the navigation sidebar anymore. The inserter of the off-canvas menu editor assumes the navigation block or a child is selected. On the navigation sidebar, the navigation block may not be selected, so we need a specific inserter for the navigation sidebar. This PR adds a simple inserter for the navigation sidebar. Given that we previously wanted to have the text "Add menu item" this PR also adds the text to the inserter superseding PR #46949.
jorgefilipecosta
force-pushed
the
fix/make-navigation-page-list-load-its-items-on-sidebar
branch
from
30afe97
to
148b2c1
Compare
jorgefilipecosta
force-pushed
the
fix/make-navigation-page-list-load-its-items-on-sidebar
branch
from
e7da8e5
to
9a060ca
Compare
jorgefilipecosta
force-pushed
the
fix/make-navigation-page-list-load-its-items-on-sidebar
branch
from
9a060ca
to
8c2f80d
Compare
| @@ -56,11 +59,20 @@ export default function NavigationMenu( { innerBlocks, onSelect } ) { | |||
| } ); | |||
| }, [ updateBlockListSettings, innerBlocks ] ); | |||
|
|
|||
| // The hidden block is needed because it makes block edit side effects trigger. | |||
| // For example a navigation page list load its items has an effect on edit to load its items. | |||
There was a problem hiding this comment.
This is unexpected for me and also raises some fundamental questions :) It seems the page list block is using "InnerBlocks" in a very unexpected way 🤔
- Is this the only place that does this: fill inner blocks with "edit" functions. I guess not, all "controlled" containers are doing this. And it raises the question of whether this should be part of "edit" function or treated as some kind of "loader" that several blocks can have.
- Also if this is some kind of "loader" that is independent of "editing" (edit function), could we leverage this loader to solve the "initial loading of site editor" issue. cc @tyxla @jsnajdr
Anyway, it doesn't seem like something that needs solving immediately, but it's an interesting question to raise.
There was a problem hiding this comment.
Yeah, it's an interesting one. I don't think the way you describe it as working is quite right though, instead it generates 'parsed' block data using the pages that exist on a site. I don't think this is a misuse in terms of inner blocks being used as a controlled component, it makes sense in a way that it's synced to an external data source, the same way as reusable blocks or template parts are.
The way list view is evolving though is as a a separate view on a block tree that can now exist without a canvas, and we do have an issue around the block's edit encapsulating too much metadata about a block (allowedBlocks is already one we know about, but there's also directInsert and renderAppender and probably other things too). I see the way page list works as similar to those issues.
And it raises the question of whether this should be part of "edit" function or treated as some kind of "loader" that several blocks can have.
Is the idea of a loader a bit like hydration of a block, so that page list data would be present at load rather than at the block's runtime?
| @@ -27,6 +27,12 @@ export default function SidebarNavigationScreenNavigationMenus() { | |||
| postId: attributes.id, | |||
| } ); | |||
| } | |||
| if ( name === 'core/page-list-item' && attributes.id && history ) { | |||
There was a problem hiding this comment.
Should we be adding a "name" check to the first case as well (check that we're clicking on navigation items) for safeties or not?
There was a problem hiding this comment.
I don't think we need a check if an item is selected and has a post type and id it is safe to assume we should move there. In the future we may have more types of items or even third parties I think just relying on kind and id should be enough.
jorgefilipecosta
deleted the
fix/make-navigation-page-list-load-its-items-on-sidebar
branch
I will work on it 👍 |
| @@ -34,6 +36,7 @@ const ALLOWED_BLOCKS = { | |||
| 'core/navigation-link', | |||
| 'core/navigation-submenu', | |||
| ], | |||
| 'core/page-list': [ 'core/page-list-item' ], | |||
There was a problem hiding this comment.
If a block list is now being rendered, you may not need the allowedBlocks hack, as the block edit functions will naturally set that.
There was a problem hiding this comment.
yes, I'm getting rid of that in #48689
Mamaduka
added
the
[Feature] Site Editor
jorgefilipecosta
added
the
Backport to WP 6.7 Beta/RC
|
Cherry-picked this PR to the wp/6.2 branch. |
ntsekouras
removed
the
Backport to WP 6.7 Beta/RC
This PR makes sure that if a menu has a page list, on the navigation sidebar, we show each item (page link) of the page list. We also include the functionality to navigate to edit a page if a page item is clicked.
This PR also reduces a lot of complexity and removes unnecessary code. With this change now, the navigation sidebar does not use a separate block editor anymore. This change also seem to fix same unexpected crashes while managing the navigation. The change breaks the inserter and there is a simple fix at #48049 that should be reviewed separately.
Testing