Problem/Motivation

Issue 1: Code for non-existent regions

Basic contains the following twig code in page.html.twig:

{% set main_menu = page.primary_menu|render %}
{% set secondary_menu = page.secondary_menu|render %}

{% if main_menu or secondary_menu %}
  <nav id="navigation" class="menu{% if main_menu %} with-primary{% endif %}{% if secondary_menu %} with-secondary{% endif %}">
    <div class="container">
      {{ main_menu }}
      {{ secondary_menu }}
    </div>
  </nav><!-- /#navigation -->
{% endif %}

But it does not implement the primary_menu or secondary_menu regions to make use of the code.

Issue 2: Menu blocks already implement the <nav> tag

Since menu blocks already implement the nav tag, it is not required in the region's wrapper html.

Proposed resolution

The simplest solution is removing the twig navigation region html and associated SASS/CSS code.

The solution I prefer is fully implementing the primary_menu or secondary_menu regions. My reasons are:

  • The Drupal core implementation templates and core front-end themes provide regions for Primary and Secondary menus
  • I've been exploring themes and had downloaded a few to look at. 3 of the 4 implemented Primary and Secondary menu regions. The 4th implemented a Navigation region. It appears to be the more common experience. Without these regions Basic may not do as well in comparisons because of a perceived deficit.
  • People coming from Drupal 7 will have an expectation of menu handling
  • It's easier deleting the regions if not required than it is adding them
  • It's a kinder starting point for novice themers
  • For experienced themers it's arguably no more of a burden than the current situation

If this is the agreed direction, the double <nav> tag issue would also be fixed.

I think there is no clear "correct" answer and discussion is warranted.

Comments

dale42 created an issue. See original summary.

dale42’s picture

Here are patches for what I envision each option looking like.

joelpittet’s picture

Would need to think about this more but my gut is to remove menu specific regions.

awasson’s picture

Version: 8.x-1.x-dev » 3.x-dev
Assigned: Unassigned » awasson
Status: Active » Needs review

I tend to agree with the idea of just removing that block of twig because it is redundant.

I'll leave this open in case anyone following cares to comment and in the next couple of days I'll remove the redundant twig code and push to dev.

Cheers,
Andrew

awasson’s picture

On further reflection, I see @dale42's point about using the primary & secondary regions. I've rolled it out in a clean site with the 3.x version and it works well. I'm leaning towards re-rolling the patch for version 3.x for this approach.

Cheers,
Andrew

awasson’s picture

StatusFileSize
new1.45 KB

Patch to include main and secondary regions

awasson’s picture

StatusFileSize
new1.76 KB

Oops... ignore patch on #6.
I forgot to edit info.yml to make it aware of the change to add the regions. Use patch #7.

Cheers,
Andrew

  • awasson committed 9d331d4e on 3.x
    Issue #2901386 by awasson, dale42: Primary/Secondary Menu Regions
    
awasson’s picture

Status: Needs review » Fixed

Done and DONE!

Thanks Dale. Applied to DEV and will tag a new release shortly.

Cheers,
Andrew

Status: Fixed » Closed (fixed)

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