Bootstrap 4 menus #328 #352
Conversation
Codecov Report
@@ Coverage Diff @@
## master #352 +/- ##
=======================================
Coverage 98.19% 98.19%
=======================================
Files 88 88
Lines 2829 2829
=======================================
Hits 2778 2778
Misses 51 51Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@PaulBrownMagic Thanks so much for the contribution! This will make a really useful addition. I have a few small change requests, but generally this is looking good.
Will you be adding flat, children and section menus also?
| @@ -0,0 +1 @@ | |||
| {% extends 'menus/bootstrap4/main_menu_dropdown.html' %} | |||
There was a problem hiding this comment.
If there's no difference between this and main_menu_dropdown.html, I think it might just be simpler to just rename main_menu_dropdown.html to main_menu.html.
| <ul class="navbar-nav mr-auto"> | ||
| {% for item in menu_items %} | ||
| <li class="nav-item {{ item.active_class }}{% if item.has_children_in_menu %} dropdown{% endif %}"> | ||
| <a href="{{ item.href }}" class="nav-link{% if item.has_children_in_menu %} dropdown-toggle" id="ddtoggle_{{ item.link_page.pk }}" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false"{% else %}"{% endif %}>{{ item.text }}{% if item.has_children_in_menu %} <span class="caret"></span>{% endif %}</a> |
There was a problem hiding this comment.
I think this would be slightly easier to follow if there were 2 if statements on this line - one within class="", and another after it to add the additional attributes.
| {% if menu_items %} | ||
| <div class="dropdown-menu" aria-labelledby="ddtoggle_{{ parent_page.pk }}"> | ||
| {% for item in menu_items %} | ||
| <a href="{{ item.href }}" class="dropdown-item{% if item.has_children_in_menu %} dropdown-toggle" id="ddtoggle_{{ item.pk }}" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false"{% else %}"{% endif %}>{{ item.text }}{% if item.has_children_in_menu %} <span class="caret"></span>{% endif %}</a> |
There was a problem hiding this comment.
We seem to have gone from having <li> elements at the top level, to not having them here. I think it might be better to add them here for consistency?
Also, I think we should end the {% if item.has_children_in_menu %} within the class="" attribute definition, and add a separate condition to add the additional attributes (as above)
There was a problem hiding this comment.
@ababic It may be a bit late ... but the bootstrap 4 docs do "recommend" the dropdown is rendered as a <div> containing <a> links.
https://getbootstrap.com/docs/4.5/components/navbar/#supported-content
<nav class="navbar navbar-expand-lg navbar-light bg-light">
<a class="navbar-brand" href="#">Navbar</a>
<button class="navbar-toggler" type="button" data-toggle="collapse" data-target="#navbarSupportedContent" aria-controls="navbarSupportedContent" aria-expanded="false" aria-label="Toggle navigation">
<span class="navbar-toggler-icon"></span>
</button>
<div class="collapse navbar-collapse" id="navbarSupportedContent">
<ul class="navbar-nav mr-auto">
<li class="nav-item active">
<a class="nav-link" href="#">Home <span class="sr-only">(current)</span></a>
</li>
<li class="nav-item">
<a class="nav-link" href="#">Link</a>
</li>
<li class="nav-item dropdown">
<a class="nav-link dropdown-toggle" href="#" id="navbarDropdown" role="button" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
Dropdown
</a>
<div class="dropdown-menu" aria-labelledby="navbarDropdown">
<a class="dropdown-item" href="#">Action</a>
<a class="dropdown-item" href="#">Another action</a>
<div class="dropdown-divider"></div>
<a class="dropdown-item" href="#">Something else here</a>
</div>
</li>
<li class="nav-item">
<a class="nav-link disabled" href="#" tabindex="-1" aria-disabled="true">Disabled</a>
</li>
</ul>
<form class="form-inline my-2 my-lg-0">
<input class="form-control mr-sm-2" type="search" placeholder="Search" aria-label="Search">
<button class="btn btn-outline-success my-2 my-sm-0" type="submit">Search</button>
</form>
</div>
</nav>
As per #328