Issue 2826 - Hamburger menu not visible on all pages

Issue 2826 - Hamburger menu not visible on all pages

[saroyanm, 2015-07-28 11:55:53]

@Thomas can you please have a look. 

I've look through the Anwiki php code and seems like the behavior was same, so
it's not a regression after migration.

[Thomas Greiner, 2015-07-29 11:07:00]

https://codereview.adblockplus.org/29322846/diff/29322847/static/js/main.js
File static/js/main.js (right):

https://codereview.adblockplus.org/29322846/diff/29322847/static/js/main.js#n...
static/js/main.js:68: function navigationClick(e)
Rename variable to "event" to (1) make it consistent with existing code, (2)
make it more descriptive and (3) to adapt to older versions of IE.

https://codereview.adblockplus.org/29322846/diff/29322847/static/js/main.js#n...
static/js/main.js:79: toggleClass(document.querySelector("header nav>ul"),
"visible");
Detail: Missing whitespaces in selector string.

https://codereview.adblockplus.org/29322846/diff/29322847/static/js/main.js#n...
static/js/main.js:79: toggleClass(document.querySelector("header nav>ul"),
"visible");
Don't forget to call `event.preventDefault()` and to return `false` to make sure
that the default action is prevented.

https://codereview.adblockplus.org/29322846/diff/29322847/static/js/main.js#n...
static/js/main.js:89: document.querySelector("nav").onclick = navigationClick;
No need to use `document.querySelector`. `document.getElementsByTagName` is
sufficient here.

[saroyanm, 2015-07-29 13:55:51]

https://codereview.adblockplus.org/29322846/diff/29322847/static/js/main.js
File static/js/main.js (right):

https://codereview.adblockplus.org/29322846/diff/29322847/static/js/main.js#n...
static/js/main.js:68: function navigationClick(e)
On 2015/07/29 11:06:59, Thomas Greiner wrote:
> Rename variable to "event" to (1) make it consistent with existing code, (2)
> make it more descriptive and (3) to adapt to older versions of IE.

Done.

https://codereview.adblockplus.org/29322846/diff/29322847/static/js/main.js#n...
static/js/main.js:79: toggleClass(document.querySelector("header nav>ul"),
"visible");
On 2015/07/29 11:06:59, Thomas Greiner wrote:
> Detail: Missing whitespaces in selector string.

Done.

https://codereview.adblockplus.org/29322846/diff/29322847/static/js/main.js#n...
static/js/main.js:79: toggleClass(document.querySelector("header nav>ul"),
"visible");
On 2015/07/29 11:06:59, Thomas Greiner wrote:
> Don't forget to call `event.preventDefault()` and to return `false` to make
sure
> that the default action is prevented.

Done.

https://codereview.adblockplus.org/29322846/diff/29322847/static/js/main.js#n...
static/js/main.js:89: document.querySelector("nav").onclick = navigationClick;
On 2015/07/29 11:06:59, Thomas Greiner wrote:
> No need to use `document.querySelector`. `document.getElementsByTagName` is
> sufficient here.

Just noticed that there are two nav elements on the page and while the header
menu is by default visible on IE8 I think we can go with this modified version
of querySelector here.

[Thomas Greiner, 2015-07-29 15:38:53]

LGTM