Improve presentation of navigation at mobile #16

djmattyg007 · 2017-02-27T10:42:05Z

Add button to show/hide menu items
Ensure navigation fills the width of the viewport by moving it out of
the main container element

djmattyg007 · 2017-02-27T10:59:22Z

I didn't realise there were tests for this. I'll take a look soon.

- Add button to show/hide menu items - Ensure navigation fills the width of the viewport by moving it out of the main container element

djmattyg007 · 2017-02-27T21:44:06Z

I've rebased my branch on top of your rebased branch, and fixed the test that I broke.

djmattyg007 · 2017-02-27T21:49:21Z

I've pushed up another commit that should resolve the line length issue. I find it odd that the line length test failed on python2 but not python3.

Thor77 · 2017-02-27T21:58:01Z

Thanks for your contribution, will start reviewing it asap :)

I find it odd that the line length test failed on python2 but not python3.

For each commit Travis creates 4 builds: Three are just running unit-tests for each supported python-version (2.7, 3.5 and 3.6) and one is running style-tests (running py2.7 because it's already available in travis containers and therefore doesn't need to be downloaded => faster build times)

Thor77 · 2017-02-27T22:03:55Z

tsstats/tests/test_template.py

@@ -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(id='main-nav').find('span').text == 'debug mode'
+    debug_element = soup.find('nav').find('div', id='main-nav').find('span')


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

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.

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.

Thor77 · 2017-02-27T22:06:10Z

tsstats/templates/index.jinja2

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


Please attach the event-listener just to the button and not to the whole document.

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

It's what the default bootstrap javascript would do if it was to be used, just with jQuery instead of vanilla javascript.

Hm, okey. Thanks for linking this article, but can you at least add it just to the nav-object instead of the whole document?

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.

Thor77 · 2017-02-27T22:10:33Z

tsstats/templates/index.jinja2

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


Is there a reason you're adding navbar-brand for a second time here?

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.

Thor77 · 2017-02-27T22:13:39Z

tsstats/templates/index.jinja2

+    var toggleTarget = document.getElementById(toggler.dataset.target);
+
+    if (toggleTarget.classList.contains('collapse')) {
+      toggleTarget.classList.remove('collapse');


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?

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.

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

djmattyg007 · 2017-03-02T12:07:31Z

I should get to these updates over the weekend.

djmattyg007 · 2017-03-06T22:28:50Z

I've just pushed the requested updates.

Thor77 · 2017-03-07T14:55:33Z

Thanks for your contribution! 👍

* add button to show/hide menu items * ensure navigation fills the width of the viewport by moving it out of the main container element

Thor77 force-pushed the bootstrap4 branch from ebcc117 to 541df38 Compare February 27, 2017 21:33

Improve presentation of navigation at mobile

ce6529b

- Add button to show/hide menu items - Ensure navigation fills the width of the viewport by moving it out of the main container element

djmattyg007 force-pushed the bootstrap4 branch from 1ae2c5b to ce6529b Compare February 27, 2017 21:37

Fix template test for debug mode

e9f1de3

Improve debug mode template test, reduce line length

87bb778

Thor77 requested changes Feb 27, 2017

View reviewed changes

djmattyg007 added 2 commits March 7, 2017 09:26

Don't use event delegation, use aria-expanded for "is collapsed" check

90f6d98

Reformat testing code

e08bbdc

Thor77 approved these changes Mar 7, 2017

View reviewed changes

Thor77 merged commit 9777f77 into Thor77:bootstrap4 Mar 7, 2017

djmattyg007 deleted the bootstrap4 branch March 8, 2017 21:40

Thor77 pushed a commit that referenced this pull request Mar 9, 2017

Improve navbar on mobile devices (#16)

9c031f9

* add button to show/hide menu items * ensure navigation fills the width of the viewport by moving it out of the main container element

Thor77 pushed a commit that referenced this pull request Aug 23, 2017

Improve navbar on mobile devices (#16)

978be86

* add button to show/hide menu items * ensure navigation fills the width of the viewport by moving it out of the main container element

Thor77 pushed a commit that referenced this pull request Sep 8, 2017

Improve navbar on mobile devices (#16)

c83f569

* add button to show/hide menu items * ensure navigation fills the width of the viewport by moving it out of the main container element

Thor77 pushed a commit that referenced this pull request Jan 19, 2018

Improve navbar on mobile devices (#16)

c85c9d0

* add button to show/hide menu items * ensure navigation fills the width of the viewport by moving it out of the main container element

Thor77 pushed a commit that referenced this pull request Jun 30, 2018

Improve navbar on mobile devices (#16)

c5fbe1d

* add button to show/hide menu items * ensure navigation fills the width of the viewport by moving it out of the main container element

Thor77 pushed a commit that referenced this pull request Mar 10, 2019

Improve navbar on mobile devices (#16)

98c1d68

* add button to show/hide menu items * ensure navigation fills the width of the viewport by moving it out of the main container element

Thor77 pushed a commit that referenced this pull request Mar 10, 2019

Improve navbar on mobile devices (#16)

c34f2d4

* add button to show/hide menu items * ensure navigation fills the width of the viewport by moving it out of the main container element

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve presentation of navigation at mobile #16

Improve presentation of navigation at mobile #16

djmattyg007 commented Feb 27, 2017

djmattyg007 commented Feb 27, 2017

djmattyg007 commented Feb 27, 2017

djmattyg007 commented Feb 27, 2017

Thor77 commented Feb 27, 2017 •

edited

Loading

Thor77 Feb 27, 2017 •

edited

Loading

djmattyg007 Feb 28, 2017

Thor77 Feb 28, 2017

Thor77 Feb 27, 2017

djmattyg007 Feb 28, 2017

djmattyg007 Feb 28, 2017

Thor77 Feb 28, 2017 •

edited

Loading

Thor77 Feb 28, 2017 •

edited

Loading

Thor77 Feb 27, 2017

djmattyg007 Feb 28, 2017

Thor77 Feb 27, 2017

djmattyg007 Feb 28, 2017

Thor77 Feb 28, 2017

djmattyg007 commented Mar 2, 2017

djmattyg007 commented Mar 6, 2017

Thor77 commented Mar 7, 2017

Improve presentation of navigation at mobile #16

Improve presentation of navigation at mobile #16

Conversation

djmattyg007 commented Feb 27, 2017

djmattyg007 commented Feb 27, 2017

djmattyg007 commented Feb 27, 2017

djmattyg007 commented Feb 27, 2017

Thor77 commented Feb 27, 2017 • edited Loading

Thor77 Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thor77 Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Thor77 Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djmattyg007 commented Mar 2, 2017

djmattyg007 commented Mar 6, 2017

Thor77 commented Mar 7, 2017

Thor77 commented Feb 27, 2017 •

edited

Loading

Thor77 Feb 27, 2017 •

edited

Loading

Thor77 Feb 28, 2017 •

edited

Loading

Thor77 Feb 28, 2017 •

edited

Loading