Make parts of the BlockNavigationList overridable using slots #21948
Conversation
adamziel
requested review from
ajitbohra,
ellatrix,
jorgefilipecosta,
Soean,
talldan and
youknowriad
as code owners
|
Size Change: +2.45 kB (0%) Total Size: 824 kB
ℹ️ View Unchanged
|
adamziel
added
[Block] Navigation
|
What's still missing here is distinguishing between the contextual/experimental block navigator and the global one - we don't want to make the text editable in the global navigator. |
| @@ -193,6 +215,11 @@ function NavigationLinkEdit( { | |||
| /> | |||
| </PanelBody> | |||
| </InspectorControls> | |||
| <BlockNavigationListItemFill blockId={ clientId }> | |||
There was a problem hiding this comment.
You may be able to avoid the need to pass the clientId here by using something like BlockListBlockContext internally in the BlockNavigationListItemFill, this seems to have the clientId:
Or there might be another way. I think all the other slots (like the sidebar and toolbar) rely on showing only content for the selected block, whereas this use case is a bit different.
We also have the same issue with some of the block toolbar items that we'd ideally not display on the block toolbar in the nav menus page - #21310 One idea might be to use an editor setting (e.g. Another option might be applying some filters to the block when registering. That approach would work well for changing the block |
adamziel
force-pushed
the
add/navigation-menu-block/allow-editing-of-new-menu-items-from-the-block-inspector
branch
from
768d9cd to
9352479
Compare
|
@talldan Super helpful notes - it thank you so much! Please keep them coming :-)
I believe the problem here is different. While #21310 is definitely relevant, here we want to make sure that the "global" navigator in post editor keeps working the same way, while pretty much every other navigator is editable:
I used contexts for now as they seem the cleanest way out. |
|
It's working, although typing in the inspector is super slow - my next step is profiling. Any suggestions and feedback are welcome! |
|
The root cause of slowness is that every time a block attribute change (e.g. label after every key stroke), we re-render block styles preview in the inspector. This is really slow! I'll look into some optimizations around these styles. Alternatively we could disable rich text editing in the inspector for now. |
|
Performance fix available in #21973 - in the meantime let's get this one reviewed :-) |
|
The performance fix is now merged. |
adamziel
force-pushed
the
add/navigation-menu-block/allow-editing-of-new-menu-items-from-the-block-inspector
branch
from
794f184 to
32543c0
Compare
|
@talldan I addressed all your feedback. Note that this feature is explicitly enabled in two inspectors in the post editor. |
Use Slot/Fill instead of hardcoded components Disable slots/fills in global editor block navigator Grab clientId from context instead of requiring a prop Improve readability of block-navigation/list.js Use context to determine whether or not given BlockNavigation may be customized with slots Restore default value for selectBlock in navigation-structure-panel.js Remove obsolete onChange passed to NavigationStructurePanel Use constant value for BlockNavigationContext Use memo() in BlockStyles Remove performance changes from BlockStyles Generalize BlockNavigationListItem and remove the BlockNavigationListItemFill for now Rename useBlockNavigationSlots to withBlockNavigationSlots Restore RichText in the navigator Sort out BlockNavigationListItem and BlockNavigationListItemWrapper Rename BlockNavigationListItemWrapper to BlockNavigationBranch
adamziel
force-pushed
the
add/navigation-menu-block/allow-editing-of-new-menu-items-from-the-block-inspector
branch
from
309c4db to
edb8c67
Compare
There was a problem hiding this comment.
Happy with this, though I noticed a few minor things I could've caught in an earlier review, so sorry about that @adamziel!
In terms of the user experience, I still have a question over how a user will go about selecting a block that's editable in the navigation. Potentially they could click the icon.
The main concern for me would be having blocks that are editable next to blocks that are not editable and how we present to the user that clicking one does something different to clicking the other.
Having said that, this is only active in an experimental block, so lets move forwards.
We may also want to do some accessibility testing on this, or maybe audit the whole thing at some point if we can get some of the other PRs merged too.
| @@ -53,7 +53,7 @@ function BlockNavigationDropdownToggle( { isEnabled, onToggle, isOpen } ) { | |||
| ); | |||
| } | |||
|
|
|||
| function BlockNavigationDropdown( { isDisabled } ) { | |||
| function BlockNavigationDropdown( { isDisabled, withBlockNavigationSlots } ) { | |||
There was a problem hiding this comment.
I think it'd be good to make this __experimentalWithBlockNavigationSlots, as it's a publicly exported prop, and it's hard to say what the future is, this may default to true at some point at which point we'd want to remove it gracefully without having to support backwards compatibility.
| const blockType = getBlockType( block.name ); | ||
|
|
||
| return ( | ||
| <div className="block-editor-block-navigation__item"> |
There was a problem hiding this comment.
I just noticed some of the class names don't seem to be right after moving the code around. The root element should have the class name block-editor-block-navigation-list-item to reflect the component name.
| export { default as __experimentalBlockNavigationList } from './block-navigation/list'; | ||
| export { | ||
| default as __experimentalBlockNavigationList, | ||
| BlockNavigationContext as __experimentalBlockNavigationContext, |
There was a problem hiding this comment.
I think this no longer needs to be exported now that it's only used internally in the BlockNavigation components
No worries at all! I just addressed all of them
Agreed, we'll get there. One option would be to just have it enabled on the experimental nav-menus screen. And even if we decide not to use RichText in the navigator in the end, these slots are going to be useful for the ellipsis menu.
👍 |
|
Looks like there are some failing e2e tests that are relevant. |
adamziel
changed the title
aristath
deleted the
add/navigation-menu-block/allow-editing-of-new-menu-items-from-the-block-inspector
branch
Description
First attempt at #20748
This PR makes it possible to plug into
BlockNavigationListusing slots.How has this been tested?
On experimental nav-menus page:
Go to
/wp-admin/admin.php?page=gutenberg-navigationEdit the text from the navigation structure panel like on the gif below:
Try with both top-level and nested navigation items
In posts editor:
Make sure to use rich formatting like pressing enter or using keyboard shortcuts (e.g. cmd+b on mac to bold selected text).
Try editing titles through the navigator in modal:
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: