-
Notifications
You must be signed in to change notification settings - Fork 2
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.
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
Improve presentation of navigation at mobile #16
Changes from 1 commit
ce6529b
e9f1de3
87bb778
90f6d98
e08bbdc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,9 @@ | |
a:hover { | ||
text-decoration: none; | ||
} | ||
.navbar-toggler { | ||
cursor: pointer; | ||
} | ||
@media screen and (max-width: 767px) { | ||
.hint--medium--xs:after { | ||
white-space: normal; | ||
|
@@ -24,23 +27,30 @@ | |
</style> | ||
</head> | ||
<body> | ||
<div class="container-fluid"> | ||
<nav class="navbar navbar-toggleable-md navbar-light bg-faded sticky-top"> | ||
<a class="navbar-brand" href="#">{{ title }}</a> | ||
<nav class="navbar navbar-toggleable-md navbar-light bg-faded sticky-top"> | ||
<div class="d-flex justify-content-between hidden-lg-up"> | ||
<a href="" class="navbar-brand">{{ title }}</a> | ||
<button class="navbar-toggler hidden-lg-up" type="button" data-toggle="collapse" data-target="main-nav" aria-controls="main-nav" aria-expanded="false" aria-label="Toggle navigation"> | ||
<span class="navbar-toggler-icon"></span> | ||
</button> | ||
</div> | ||
|
||
<div class="collapse navbar-collapse"> | ||
<ul class="navbar-nav mr-auto"> | ||
{% for sid, _ in servers %} | ||
<li class="nav-item"> | ||
<a class="nav-link" href="#sid{{ sid }}">Server {{ sid }}</a> | ||
</li> | ||
{% endfor %} | ||
</ul> | ||
{% if debug %} | ||
<span class="navbar-text" style="color: red">debug mode</span> | ||
{% endif %} | ||
</div> | ||
</nav> | ||
<div id="main-nav" class="collapse navbar-collapse"> | ||
<ul class="navbar-nav mr-auto"> | ||
<li class="hidden-md-down"><a href="" class="navbar-brand">{{ title }}</a></li> | ||
{% for sid, _ in servers %} | ||
<li class="nav-item"> | ||
<a class="nav-link" href="#sid{{ sid }}">Server {{ sid }}</a> | ||
</li> | ||
{% endfor %} | ||
</ul> | ||
{% if debug %} | ||
<span class="navbar-text" style="color: red">debug mode</span> | ||
{% endif %} | ||
</div> | ||
</nav> | ||
|
||
<div class="container-fluid"> | ||
{% for server in servers %} | ||
<h1 class="display-4" id="sid{{ server.sid }}"> | ||
<a href="#sid{{ server.sid }}">Server {{ server.sid }}</a> | ||
|
@@ -49,5 +59,23 @@ | |
{% endfor %} | ||
<small>Generated by <a href="https://github.com/Thor77/TeamspeakStats" rel="noopener">TeamspeakStats</a> at {{ creation_time|frmttime }}</small> | ||
</div> | ||
|
||
<script type="text/javascript"> | ||
document.addEventListener('click', function(event) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please attach the event-listener just to the button and not to the whole document. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm using event delegation: https://davidwalsh.name/event-delegate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's what the default bootstrap javascript would do if it was to be used, just with jQuery instead of vanilla javascript. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, okey. Thanks for linking this article, but can you at least add it just to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thought about it for a while now and I don't think we should use event delegation here, as we only need one eventlistener and are not dynamically modifying the DOM. Therefore we don't use any of the benefits of event-delegation. |
||
var toggler = event.target; | ||
if (toggler.dataset.toggle !== 'collapse') { | ||
return; | ||
} | ||
var toggleTarget = document.getElementById(toggler.dataset.target); | ||
|
||
if (toggleTarget.classList.contains('collapse')) { | ||
toggleTarget.classList.remove('collapse'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh, please don't mess with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there's a good answer to this, as there's nothing to stop the presence of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer to just rely on |
||
toggler.setAttribute('aria-expanded', false); | ||
} else { | ||
toggleTarget.classList.add('collapse'); | ||
toggler.setAttribute('aria-expanded', true); | ||
} | ||
}); | ||
</script> | ||
</body> | ||
</html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you're adding navbar-brand for a second time here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One is used for mobile and tablet, the other is used for desktop. The new navbar markup means it's much easier to do this than to try and only have one
navbar-brand
element.