Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bootstrap 4 menus #328 #352

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Bootstrap 4 menus #328 #352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91dcbc9
Bootstrap4 Templates
PaulBrownMagic Aug 30, 2019
54be004
Correct class handling
PaulBrownMagic Aug 30, 2019
79bc51b
Correct class on a
PaulBrownMagic Nov 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions wagtailmenus/templates/menus/bootstrap4/main_menu.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{% extends 'menus/bootstrap4/main_menu_dropdown.html' %}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

11 changes: 11 additions & 0 deletions wagtailmenus/templates/menus/bootstrap4/main_menu_dropdown.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{% load menu_tags %}
<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>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 item.has_children_in_menu %}
{% sub_menu item template="menus/bootstrap4/sub_menu.html" %}
{% endif %}
</li>
{% endfor %}
</ul>
11 changes: 11 additions & 0 deletions wagtailmenus/templates/menus/bootstrap4/sub_menu.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{% load menu_tags %}
{% 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>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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>

{% if item.has_children_in_menu %}
{% sub_menu item template=current_template %}
{% endif %}
{% endfor %}
</div>
{% endif %}