Add the new Navigation module to core

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.

We're still working on cleaning-up the MR and providing the necessary info on the issue summary, so leaving this as Active for now to finish those. When it's done we'll move this to Needs Review.

Moving to Needs Review to start gathering reviews and feedback :)

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.

Adding some design system related info as context.

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 📌 Convert navigation sections to blocks and use the menu system Active 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.

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

Beyond the code and on a UX perspective, the decision to have an independent page to manage the blocks for the admin navigation was deliberate. We wanted a separated page because:

1. Appears both in front-end and admin themes (as summarized in #18)
2. Because it's a different context: you are customizing the "admin UI", not "the site". So you are not changing the site Structure (where Block layout is placed), but the admin tools you are providing to work on the site.

We wanted to reuse something that existing devs know so they can create and place here custom blocks specifically created for the navigation (so they don't place anything and breaks the admin navigation). The result has been duplicated code, which is not ideal but a requirement of being a contib as mentioned. In an ideal world we would change block in core so the changes can be used also to eventually place blocks in a future Dashboard 🌱 Enhance user experience with customizable dashboards Active . But realistically we probably can't change core in 1 week: so does it mean we should try to get in as alpha?

m4olivei → closed merge request !7474

m4olivei → reopened merge request !7474

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

ckrina → changed the visibility of the branch 3438895-module-flag to hidden.

catch → changed the visibility of the branch 3438895-attempt-to-remove-duplication to hidden.

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!

Agh! I forgot to reference 📌 Investigate using the core "User account menu" in favor of custom Navigation Block for same Active when I closed the thread, but yeah “someone might want to add or remove things” is a really good reason to rethink the strategy.

So we do have a follow-up so moving it to Needs Review again :)

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! 🎉

Tagging with "Needs frontend framework manager review" to make visible a missing step.

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.

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

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.

Yes, that would be the place since it's expected to handle contextual actions needed for the current page.

I've just created a CR but I haven't said anything about the Top Bar because it's only on its early stage, so I don't want to encourage people to enable it. For those with more experience with CR, does this makes sense?

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.

Updated the release notes section.

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 →

Since I've been mostly doing code review during the initiative and all the front-end related issues found on the MR have been either addressed here or follow-ups have been opened, I feel comfortable removing the "Needs frontend framework manager review" tag.

lauriii → credited chetan 11 → .

lauriii → credited claireristow → .

lauriii → credited ctrlADel → .

lauriii → credited dead_arm → .

lauriii → credited DieterHolvoet → .

lauriii → credited Emma Horrell → .

lauriii → credited flanneryla → .

lauriii → credited Gábor Hojtsy → .

lauriii → credited gaurav_manerkar → .

lauriii → credited gdd → .

lauriii → credited gnuschichten → .

lauriii → credited hooroomoo → .

lauriii → credited hot_sauce → .

lauriii → credited iro → .

lauriii → credited javi-er → .

lauriii → credited joaopauloc.dev → .

lauriii → credited johnpicozzi → .

lauriii → credited jponch → .

lauriii → credited jwilson3 → .

lauriii → credited jwitkowski79 → .

lauriii → credited Kanchan Bhogade → .

lauriii → credited KeyboardCowboy → .

lauriii → credited kostyashupenko → .

lauriii → credited KurtTrowbridge → .

lauriii → credited marcelreimer → .

lauriii → credited markie → .

lauriii → credited Meeni_Dhobale → .

lauriii → credited Mithun S → .

lauriii → credited NikMis → .

lauriii → credited pjudge → .

lauriii → credited Prashant.c → .

lauriii → credited q0rban → .

lauriii → credited rabbitlair → .

lauriii → credited rkoller → .

lauriii → credited sani → .

lauriii → credited saschaeggi → .

lauriii → credited Shreya_th → .

lauriii → credited silviu → .

lauriii → credited Silviu S. → .

lauriii → credited SKAUGHT → .

lauriii → credited starshaped → .

lauriii → credited Svitlana → .

lauriii → credited tedbow → .

lauriii → credited tim.plunkett → .

lauriii → credited Utkarsh_33 → .

lauriii → credited vensires → .

lauriii → credited YurkinPark → .

I opened 📌 Mark Navigation as beta experimental Active 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! 🎉

lauriii → credited bnjmnm → .

lauriii → credited bronzehedwick → .

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 📌 Mark Navigation as beta experimental Active 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.

+1

Mithun S → changed the visibility of the branch 3438895-add-the-new to hidden.

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

Automatically closed - issue fixed for 2 weeks with no activity.

Adding my organization