| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Try adding content roles to navigation blocks #71747
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.
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
Conversation
tellthemachines
requested review from
aaronrobertshaw,
andrewserong,
getdave,
ramonjd,
scruffian and
talldan
tellthemachines
requested review from
ajitbohra,
fabiankaegy and
juanmaguitar
as code owners
tellthemachines
added
[Type] Enhancement
|
Flaky tests detected in 7dd8d6b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17848996372
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is testing as advertised for me.
✅ Tested with the Navigation block as both the root and child of a pattern
✅ Adding buttons continues to work
✅ Adding Navigation Links and Submenus works
LGTM 🚢
Screen.Recording.2025-09-19.at.4.48.28.pm.mp4
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Related #71137 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great and I also rebased #71137 and included the same set of changes there.
The only concern I have is that this change will effectively "enable" the Nav block in contentOnly. As it stands the experience isn't great so I'd be holding back on shipping these changes.
However, as we know the aim is to develop a solid Nav block experience in contentOnly this may act as a forcing function.
Overall I'm in favour of shipping and iterating.
I'm not too concerned because contentOnly is behind a feature flag, so only testers will have access to it. Also, I mostly made this PR to test how it feels right now, and I think it's better than it was a month or so ago, with all the improvements we've made in the meantime (at least now we can delete nav items as well as add them! 😅) The only thing that's still suboptimal IMO is Page list not being editable. I might try adding a contentRole to it too, and see if that works better. |
|
I'm noticing Page list has a slightly different behaviour from Nav link in that, if it's the only block in the Nav it's pretty much impossible to select the Nav, and no inserter appears, whereas if the Nav link is the only block in the Nav the inserter does appear. I'm not sure if that's deliberate or not, but in contentOnly mode it makes it harder to edit a Nav that only has a Page list in it. This still feels like an overall improvement compared to current behaviour, so I'll go ahead and merge in the next day or so unless anyone objects. |
|
Is contentOnly behind a feature flag? I thought that was Write Mode. My understanding (which may be wrong!) was that block editing mode is a public API. I don't want to split hairs here but - assuming I'm correct - it's something to consider. |
Both write mode and contentOnly applied to patterns by default are behind flags. That only leaves contentOnly as a templateLock setting, and that one doesn't work quite the same (it's not possible to add blocks at all if templateLock is set to contentOnly) In practice, the only chance of this change affecting production sites is allowing people to edit any existing nav links that are inside a block with templateLock. |
|
Ah yes. I had the experiment on. That was it. Thanks for confirming. |
Labels
3 participants
What?
Works towards #65699.
I wanted to see how far we can go with just adding content roles to Nav, Nav link and Submenu blocks, given the improvements to contentOnly logic we've had since #70977.
I think it's working better now! At least we're able to add and delete items 😅
I purposely haven't made any changes to the inspector because more wide-ranging inspector changes are in progress in #71730.
Currently if the Nav only contains a Page List block, we're not able to do anything with it. This could be solved by adding "contentRole" to the Page List too.
Testing Instructions
The Nav in the screenshot has a Buttons block in it, but in contentOnly mode it's not possible to add any non-link or non-submenu blocks. If the Buttons is there you can add more Button blocks inside it though.