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

Jump to bottom

Improve presentation of navigation at mobile #16

Merged
merged 5 commits into from
Mar 7, 2017
Merged

Improve presentation of navigation at mobile #16

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce6529b
Improve presentation of navigation at mobile
djmattyg007 Feb 27, 2017
e9f1de3
Fix template test for debug mode
djmattyg007 Feb 27, 2017
87bb778
Improve debug mode template test, reduce line length
djmattyg007 Feb 27, 2017
90f6d98
Don't use event delegation, use aria-expanded for "is collapsed" check
djmattyg007 Mar 6, 2017
e08bbdc
Reformat testing code
djmattyg007 Mar 6, 2017
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
60 changes: 44 additions & 16 deletions tsstats/templates/index.jinja2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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>
Copy link
Owner

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?

Copy link
Contributor Author

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.

{% 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>
Expand All @@ -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) {
Copy link
Owner

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using event delegation: https://davidwalsh.name/event-delegate

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Owner

@Thor77 Thor77 Feb 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The 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 nav-object instead of the whole document?

Copy link
Owner

@Thor77 Thor77 Feb 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The 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');
Copy link
Owner

Choose a reason for hiding this comment

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

Huh, please don't mess with the collapse-class here. You can check the current collapse-state via the aria-expanded-attribute, can't you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 collapse class and the value of the aria-expanded attribute from getting out of sync. I'm happy to test whichever one you prefer, but it is easier to test for the presence of the classname IMO.

Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer to just rely on aria-expanded, because adding/removing classes just for saving a state seems kinda wrong to me.

toggler.setAttribute('aria-expanded', false);
} else {
toggleTarget.classList.add('collapse');
toggler.setAttribute('aria-expanded', true);
}
});
</script>
</body>
</html>
3 changes: 2 additions & 1 deletion tsstats/tests/test_template.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ def test_debug(output):
logger.setLevel(logging.INFO)
soup = BeautifulSoup(open(output_path), 'html.parser')
# check debug-label presence
assert soup.find('nav').find('span').text == 'debug mode'
debug_element = soup.find('nav').find('div', id='main-nav').find('span')
Copy link
Owner

@Thor77 Thor77 Feb 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

You could've just split the line with a backslash (\) before the== but you can keep it this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't think there was a neat place to split the line using a backslash. Any positioning would have made the code far less readable than just keeping it all on one line IMO.

Copy link
Owner

Choose a reason for hiding this comment

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

Nah, would've looked like this with splitting by backslash:

assert soup.find('nav').find(id='main-nav').find('span').text \
    == 'debug mode'

Don't think that's far less readable.

assert debug_element.text == 'debug mode'
for client_item in soup.find('ul', id='1.onlinetime').find_all('li'):
nick = client_item.find('span').text
# check for right identifier
Expand Down