Problem/Motivation

The experimental Navigation module has some code that hides the current toolbar module when both are enabled.

Because we are going to deprecate toolbar module #3484850: [meta] Tasks to deprecate Toolbar module, we should move this code from navigation to toolbar as part of the process of marking navigation stable. We might be able to improve things by preventing the toolbar from being built in the first place too.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3507711

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

quietone made their first commit to this issue’s fork.

gábor hojtsy made their first commit to this issue’s fork.

gábor hojtsy changed the visibility of the branch main to hidden.

gábor hojtsy opened merge request !15662

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Active » Needs work

The actual code removing toolbar is in NavigationRenderer. I think doing similar in toolbar module would be simpler as it would just skip doing anything assuming Navigation module is enabled?

/**
   * Remove the toolbar provided by Toolbar module.
   *
   * @param array $page_top
   *   A renderable array representing the top of the page.
   *
   * @see toolbar_page_top()
   * @see hook_page_top()
   */
  public function removeToolbar(array &$page_top): void {
    if (isset($page_top['toolbar'])) {
      unset($page_top['toolbar']);
    }
  }

The requirements hook is already moved in the MR, but this is still outstanding.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

I tested this out pretty basically. With the MR applied I installed navigation and toolbar is hidden. So nothing seemed to break.

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs review

Can we test that the caching works correctly?

Install toolbar and navigation.
Test with user who does not have access navigation. Then add that permission to the user and test again.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Okay trying to address #10

I created a test user (test.editor)
Revoked the access navigation permission
Logged in a separate browser and verified I don't see the navigation bar
Will note I also don't see toolbar

I added the permission
Refreshed the other browser with test.editor and see the bar now

So believe this is working.

catch closed merge request !15662

  • catch committed cef7fc97 on 11.x 
    task: #3507711 Move the code that hides the toolbar when navigation is...

  • catch committed a1b8f1ae on main 
    task: #3507711 Move the code that hides the toolbar when navigation is...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and cherry-picked to 11.x., thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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