Add the new Navigation to core as an Experimental module

A couple of handy references for our purposes here:

• Claro issue used to prepare for core inclusion. A patch was managed here for the changes needed between the contrib version of the module and core. We'll need something similar here.
• The script that was used by Claro for trasposing the contrib version of the theme into a core patch. Provided by @laurii. We'll need something similar here.

I'm going to work on the script a little.

a bunch of comments, not found of using custom event, I'd subclass it and call the new class directly

Not a full review but a handful of comments on the MR.

gonna do small fixes right in MR

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I've collated the duplication between the navigation module and the block module.

Is there any of this we can reuse some of the things in core now we have a core MR and the ability to make changes to core to support our needs?

@larowlan Thank you for opening that discussion.

We need at some point to address the code duplication issue.

I'm not sure whether this is a task to work on now or when moving the module from experimental to stable.

If I understood correctly, experimental modules recommendation was to not modify other core pieces just in case they will not be finally promoted to stable. Not sure if that's not actually the case, or if this scenario could be an exception.

If I understood correctly, experimental modules recommendation was to not modify other core pieces just in case they will not be finally promoted to stable.

Navigation supplying plugin implementations to eventually move to other modules (as is in the MR) is good because then if there was removal later, it's a clean break. That's what's meant by modifying core pieces.

However core MRs to improve APIs, so that the experimental module doesn't have to work around them - these are always fine. Workspaces for example had to make dozens of changes to the entity API and individual core entity types (revision support, publishing support etc.) before it was stable, but all of those made sense if workspaces never existed too.

I think the main question with the duplication, is whether the separate config storage is definitely going to be maintained or not, if it is, then it's mostly a matter of code duplication, if it was somehow going to be merged with block configuration, then it mean an upgrade path and block beta (but not necessarily alpha).

I think https://www.drupal.org/project/navigation/issues/3397058#comment-15331899 describes why we walked away from blocks - that the Navigation toolbar will never have a true render context in build() and will never be tied to a theme, whereas blocks are always placed within a theme.

Closed / re-opened the MR to trigger Tugboat. Sorry for the noise.

I've opened two new MRs as follows

• https://git.drupalcode.org/project/drupal/-/merge_requests/7644 (against core)
• https://git.drupalcode.org/issue/drupal-3438895/-/merge_requests/1 (against original branch)

They're ready for review and testing, screenshots on the MR - seems to be working fine, net change is -4000 lines of code

I didn't want to push to the existing branch as I felt folks might want to review the alternate (layout builder) approach. Check the second of those MRs above for the change between both approaches.

@larowlan i've tested LB approach in terms of frontend. Looks fine and i didn't find regressions.

Probably we need to use new page with admin UI for this layout edition.
Because on mobile or in collapsed state it became hard to use.

Also we have styled block titles in blocks approach. Please check as well. Thanks

Let's assume this will get in and tag it for release highlights :)

From a framework manager point of view, I'm happy to sign off on the backend code in this MR for the following reasons:

• There is no new API, it is purely a feature module making use of existing Drupal hooks and plugins
• The changes are entirely isolated to the module, it can be uninstalled if it causes any issues on your site without any data-model bleeding into the site
• There is only simple config stored so if we ever need an update path for API changes, updating 2 simple config objects feels much less risky than e.g. updating content entities

I'd like to congratulate everyone who worked on it, its an awesome piece of functionality and a huge step forward for Drupal core

For the open items in the MR

Frontend feedbacks resolved

Tests are green and all the MR threads are closed.

Moving to Needs Review again for a new round of reviews.

Thanks to everyone who has worked to make it possible!

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

This is so beautiful. Everyone has done (and is doing) amazing work!

• I pulled down the diff and applied it against a fresh 11.x install.
• I tested out the functionality and it works as expected, and as intended.
• It looks absolutely so beautiful.
• I was able to effectively navigate around via keyboard
• I tested in forced colors mode, and although there are are some minor issues, the menu is perceivable, operable, understandable, and robust.
• All threads appear to be resolved.
• I did not fully review the code, although it appears that it has been thoroughly reviewed. I will spend more time on this after it enters beta

RTBC! 🎉

Took this for a very quick test drive and went to what I think it the hardest admin page we have... the node preview. Which looks fantastic apart from 1 very important detail. We're missing the vital "Back to content editing" button. See screenshots below:

Steps to reproduce:

1. Install standard
2. Install navigation module
3. Visit node/add/article
4. Fill in required field
5. Press the preview button

With toolbar

With navigastion

The button is there! It is just hidden by the navigation :)

Gonna hotfix

as agreed back to rtbc. also fixed module css. to fix umami preview.

Nice fix. So frontend themes might need to make changes to support the module. I think we need to document what changes they need to make in a change record attached to this issue. Looking at the MR changes this looks like the only one a frontend theme might have to make.

Took this for a test drive too and it really looks great - minor points below:

Definitely follow-up material, but I just tried installing navigation module with toolbar module already installed to see if it looked broken, and it works great... presumably we're altering away toolbar, think I saw that in the MR.

Navigation and toolbar will both need to be stable in core simultaneously for at least some amount of time, so I think we should change (or add to) the alter with an explicit hook_requirements() implementation to recommend uninstalling toolbar when navigation is installed. Maybe in navigation while it's in beta and in toolbar when navigation is stable and toolbar is deprecated?

On my 13" laptop monitor, the menu scrolls and the user/help section does not. I guess that's by design, but I kind of wondered if I could collapse only the footer so that I could fit the whole menu vertically.

I also wonderd if the top bar would handle the node preview back to content editing button issue that @alexpott brought up, but because it's not a local task or local action, it can't do it the same way it does node local tasks. Might be a possible approach though.

(lol I tried to post this and it failed, because that's already been fixed, making the preview slide over works too).

Thank you for your reviews.

Navigation already provides a basic hook_requirements() implementation: https://git.drupalcode.org/project/drupal/-/merge_requests/7474/diffs#80...

If you think it can be improved, or is not descriptive enough, we could update it.

Ah I didn't spot that, but yes it's there in the status report. I would maybe change it to 'Toolbar and Navigation modules are both installed' then something like 'The Navigation module is a complete replacement for the Toolbar module and disables its functionality when both modules are installed. If you are planning to continue using Navigation module, you can uninstall the toolbar module now"? But even more follow-up material given it already exists.

Oh one more thing, I just tried out the layout builder configuration which I felt bad for even suggesting in the first place. It took me a few seconds to realise that the layout configuration was in the actual navigation itself (was expecting the usual full page layout), but as soon a I realised, my mind was blown.

Tried messing around with the block ordering, and was able to make everything fit on my laptop without scrolling by dropping the logo and the shortcuts menu item.

This took about 45 seconds, can see dozens of possibilities here, really nice and what a difference from the toolbar!

So glad the half-baked suggestion actually worked.

RE #44

So frontend themes might need to make changes to support the module.

Here we have case when not all modules and themes supports Drupal.displace, it is not related to Navigation module at all.

This preview panel also has problems with another displace elements. Also fixed in last commit.

Fixes in #41 are correct, but do not take into account RTL because it's using logical properties with Drupal displace (which is not language direction aware).

Fix incoming in a couple min.

RTL issue with node preview container is fixed. Good to go!

We also need a CR for themers that implement a frontend theme that overrides .node-preview-container like Olivero does and to tell them how to change it so it works with the Navigation module and the Annoucements module.

@alexpott so maybe revert last fixes? they are really not about Navigation module. as described in #50

I would argue that it doesn't need a CR (although if you disagree I'm happy to do it). This was a legitimate issue that pre-existed both the announcements and navigation modules. The same issue would affect anything using the off-canvas dialog, or any other component that uses drupal.displace() and the node preview container.

Change record created at https://www.drupal.org/node/3443793

Note that we had a bit of discussion in Slack at https://drupal.slack.com/archives/C7AB68LJV/p1714124756817409

I opened #3443843: Mark Navigation as beta experimental to decide on beta stability, so that we can commit the module as alpha before that. I noticed that there are some comments missing, and other minor things that could be improved. I don't think these need to block the commit because it would be much easier to fix these outside of this massive MR, especially given that the contrib module is now out of sync.

Committed dae3853 and pushed to 11.x. Congrats everyone! 🎉

Note that this only goes in the release highlights if it gets to beta and is therefore in the tagged release, so we can tag that issue when it lands. :)

Moving to 'to be ported' for 10.3.x. It needs #3443843: Mark Navigation as beta experimental to be decided first. But also if we're not going to backport this to 10.3, we need a 10.4 branch asap so we can backport it and any commits to that branch - otherwise we're going to have a complicated rebase/cherry-pick sequence to deal with.

Absolutely fantastic to see this get committed 🎉

I'm proud to have played a small apart in this! A huge achievement by all involved, well done!

I've cherry-picked this (had to resolve some merge conflicts, hopefully did that correctly) to 10.3.x along with all the other issues committed for navigation, so that even if we decide not to release it as beta in 10.3.x, we can branch 10.4.x with navigation included and delete it with a single rm -rf from 10.3. Or if it goes in as beta to 10.3.x, then it'll be ready to go without the cherry-picks blocking a beta.

Retrospective review of the 10.3 commit to make sure I didn't mess up the merge conflicts would be welcome.

Amazing good job +1

Amazing, happy to see this moving to the core. Well done!

Adding my organization

Fixing attribution.