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

Fix: Make navigation page list load its items on navigation sidebar. #47853

Merged
merged 5 commits into from
Feb 27, 2023
Merged

Fix: Make navigation page list load its items on navigation sidebar. #47853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f10f121
Fix: Make navigation page list load its items on navigation sidebar.
jorgefilipecosta Feb 7, 2023
85749c0
Second approach using a single block editor.
jorgefilipecosta Feb 13, 2023
148b2c1
Load first menu item in a new block editor if navigation is not in ca…
jorgefilipecosta Feb 23, 2023
f3f7766
revert
jorgefilipecosta Feb 23, 2023
8c2f80d
remove unrequired changes
jorgefilipecosta Feb 23, 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
22 changes: 17 additions & 5 deletions packages/edit-site/src/components/navigation-inspector/navigation-menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import {
privateApis as blockEditorPrivateApis,
store as blockEditorStore,
BlockList,
BlockTools,
} from '@wordpress/block-editor';
import { useEffect } from '@wordpress/element';
import { useDispatch } from '@wordpress/data';
Expand Down Expand Up @@ -34,6 +36,7 @@ const ALLOWED_BLOCKS = {
'core/navigation-link',
'core/navigation-submenu',
],
'core/page-list': [ 'core/page-list-item' ],
Copy link
Contributor

@talldan talldan Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If a block list is now being rendered, you may not need the allowedBlocks hack, as the block edit functions will naturally set that.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I'm getting rid of that in #48689

};

export default function NavigationMenu( { innerBlocks, onSelect } ) {
Expand All @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@talldan talldan Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

return (
<OffCanvasEditor
blocks={ innerBlocks }
onSelect={ onSelect }
LeafMoreMenu={ LeafMoreMenu }
/>
<>
<OffCanvasEditor
blocks={ innerBlocks }
onSelect={ onSelect }
LeafMoreMenu={ LeafMoreMenu }
/>
<div style={ { display: 'none' } }>
<BlockTools>
<BlockList />
</BlockTools>
</div>
</>
);
}
8 changes: 7 additions & 1 deletion packages/edit-site/src/components/sidebar-navigation-screen-navigation-menus/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export default function SidebarNavigationScreenNavigationMenus() {
const history = useHistory();
const onSelect = useCallback(
( selectedBlock ) => {
const { attributes } = selectedBlock;
const { attributes, name } = selectedBlock;
if (
attributes.kind === 'post-type' &&
attributes.id &&
Expand All @@ -27,6 +27,12 @@ export default function SidebarNavigationScreenNavigationMenus() {
postId: attributes.id,
} );
}
if ( name === 'core/page-list-item' && attributes.id && history ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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 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.

history.push( {
postType: 'page',
postId: attributes.id,
} );
}
},
[ history ]
);
Expand Down