Closed (fixed)
Project:
Basic
Version:
3.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Reporter:
Created:
10 Aug 2017 at 21:24 UTC
Updated:
7 Aug 2025 at 19:54 UTC
Jump to comment: Most recent, Most recent file
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.
Since menu blocks already implement the nav tag, it is not required in the region's wrapper html.
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:
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.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | basic-menu-regions-2901386-7-menu-region.patch | 1.76 KB | awasson |
| #2 | basic-menu-regions-2901386-2-no-nav.patch | 2.08 KB | dale42 |
Comments
Comment #2
dale42Here are patches for what I envision each option looking like.
Comment #3
joelpittetWould need to think about this more but my gut is to remove menu specific regions.
Comment #4
awasson commentedI 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
Comment #5
awasson commentedOn 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
Comment #6
awasson commentedPatch to include main and secondary regions
Comment #7
awasson commentedOops... 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
Comment #9
awasson commentedDone and DONE!
Thanks Dale. Applied to DEV and will tag a new release shortly.
Cheers,
Andrew