Twenty Twelve: improve navigation to be mobile-first and not rely on jQuery for hiding and showing

[cfinke]

When loading Twenty Twelve on an iPhone, the "Show navigation" button is visible even when tapping it will have no effect. It should be hidden (or never output) if it will be impotent.

[cfinke]

Hides the 'Show navigation' button if there are no navigation menu items.

[cfinke]

Correction: This applies to all small screens (not just iPhones), including narrow browser windows on full size screens.

[obenland]

This should only happen, when an empty menu was assigned to it, right?

[obenland]

This is so lame in so many ways.

A user should be able to "remove" the menu by assigning an empty one to it. With no menu items, we are left with some margin and borders and the mentioned button on screens < 600px.

Unless I'm missing something big, there is no way to address it with CSS. Removing it with JS, leaves us with a button/border flash on pageload though. Did I say it was lame?

[obenland]

After spending like 3h trying to find the best way to fix this issue, 21562.2.diff​ and #21576 are what I came up with.

We can loose the functions.php callback once #21576 is in core. Until then, this is the only possibility I could think of, to prevent a markup flash and not have the margin and the menu borders collapse on the empty menu.

In theme.js I just wrapped most of the JS with the menu-item-check from @cfinke and changed the window check to check for true instead of not false.

[lancewillett]

Replying to obenland:

I think going with cfinke's patch will work short-term. Just making sure the button isn't there. Yes, the empty menu assigned case is possible, but so is someone forking the theme and not showing a menu output at all (or a plugin doing that for some reason).

So the JS fix makes sense to avoid showing the button.

Let's wait until #21576 is fixed until worrying about the extra markup in the source.

[lancewillett]

In [21520]:

Twenty Twelve: selector for navigation menu should style the menu list element, props obenland, see #21562.

Also remove duplicate line-height rule, props bradthomas127. See #21577.

[lancewillett]

With r21520 I also committed the JS fix props cfinke. Forgot to note that in the commit message.

[lancewillett]

@cfinke A note on core patches, they should be made from WP root -- not the theme directory. Thanks. :)

[lancewillett]

Patch by obenland - 21562.css-menu-switch.diff - takes out most of the JS for the navigation handling and puts it into CSS with the display depending on screen size via media queries.

[lancewillett]

@obenland One thing I noticed right away -- the small menu styles are broken. Previously since the class changed to "main-small-navigation" the smaller menu did not inherit and need to override any of the "main-navigation" styles.

Small menu is lowercase, underlined, much less space in between, centered, and no sub-level menus at all (just top level).

It's all doable, just makes the small menu CSS rules a bit more complex since they have to override the larger styles. Which brings up something -- this theme is all mobile first, so we should really do it the other way around:

1. Outside media queries put small menu styles
2. Inside min-width of 600px put the larger menu styles

That takes care of the overrides and also works with how Drew built the stylesheet.

[obenland]

Great feedback! You're absolutely right. I'll give it another spin. Thanks!

[obenland]

21562.css-menu-switch.2.diff​ moves the menu-switch logic from JS to CSS. It maintains the mobile-first approach by moving the menu styles for larger screens in the >600px media query.

Renamed theme.js to navigation.js, as discussed with @lancewillett in IRC, since it is solely about the main navigation.

[lancewillett]

Replying to obenland:

21562.css-menu-switch.2.diff​ moves the menu-switch logic from JS to CSS. It maintains the mobile-first approach by moving the menu styles for larger screens in the >600px media query.

Looks much better -- thank you for the updated patch. Will get it in soon, after more testing.

[lancewillett]

Renaming ticket for accuracy, the main thrust of this ticket is the JS -> CSS rework for hiding and showing and making the styles mobile first.

[lancewillett]

Added menu-toggle class in header.php, minor cleanup

[lancewillett]

In [21554]:

Twenty Twelve: improve navigation to be mobile-first and not rely on JS for hiding and showing, props obenland for patches. See #21562.

[lancewillett]

This needs some more testing in all devices and browsers before closing.

[lancewillett]

In [21557]:

Twenty Twelve: re-implement the hiding of toggle element when empty menu is present, see #21562 and r21520 props cfinke.

[nacin]

the centering and tons of padding seem like an ineffective use of space

[nacin]

the headline and image are a hit close together here

[lancewillett]

In [21562]:

Twenty Twelve: minor style fixes for small navigation menu (line-height) -- see #21562; featured image on homepage timeplate (space below image in small layouts).

[lancewillett]

In [21564]:

Twenty Twelve: in small menu give sub-menu items a bit of indentation to denote the hierarchy of items. See #21562.

[lancewillett]

In [21573]:

Twenty Twelve: s/Show navigation/Show menu/ in small menu button text, props nacin. See #21562.

[spencerdahl]

Is it possible to change 'Show Menu' to 'Hide Menu' when the menu is visible?

[lancewillett]

Replying to spencerdahl:

Is it possible to change 'Show Menu' to 'Hide Menu' when the menu is visible?

I'd prefer not to change the text label because the general use case is:

1. Click it to view menu
2. Click a link
3. Go to new page, menu is closed again

Rarely would you need to open, then close it again (other than in testing).

[obenland]

Replying to lancewillett:

Replying to spencerdahl:

Is it possible to change 'Show Menu' to 'Hide Menu' when the menu is visible?

I'd prefer not to change the text label

+1

[obenland]

@lancewillett:

In 21562.no-jQuery.diff​ the toggling works but currently without transition effects.

CSS transitions do not work with the display property, which is why I went with height and overflow:hidden;. But they also need a specific hight value for them to work. It does not work with height:auto; (or height:100%; for that matter).

Which brings us back to: ...calculating the height??

[lancewillett]

Replying to obenland:

@lancewillett:

In 21562.no-jQuery.diff​ the toggling works but currently without transition effects.

I think we can remove the entire JS file now.

• Use hover style to reveal the menu when you mouse over the button, just like core menus work.
• Use transition to make the reveal be slower, if desired (might not even be necessary).

[lancewillett]

Replying to lancewillett:

Replying to obenland:

@lancewillett:

In 21562.no-jQuery.diff​ the toggling works but currently without transition effects.

I think we can remove the entire JS file now.

Scratch that idea, we can't rely on :hover pseudo element support in touch devices. The click to show still needs to happen.

[obenland]

What it comes down to is having the transition effect or not, right?

[Jayjdk]

Replying to obenland:

In 21562.no-jQuery.diff​ the toggling works but currently without transition effects.

Doesn't that patch assume that the ID of the menu is menu-menu (var menu = document.getElementById( 'menu-menu' ))?

That is not always the case. If I call my menu foo, the ID will be menu-foo

[obenland]

Replying to Jayjdk:

Doesn't that patch assume that the ID of the menu is menu-menu (var menu = document.getElementById( 'menu-menu' ))?

That is not always the case. If I call my menu foo, the ID will be menu-foo

That's right. The patch is a work in progress, as it also lacks support for wp_page_menu().

Which brings up a whole different topic of having to add a callback to 'wp_page_menu' to wrap it with an element with an id, to be able to target it with JS.

[lancewillett]

Replying to obenland:

What it comes down to is having the transition effect or not, right?

Yep. I think for older browsers no effect is fine — just hide and show it with CSS when they hover or click on the menu-toggle button.

For selecting the menu by ID without jQuery, something like this might work:

var menu = document.getElementById( 'masthead' ).getElementsByTagName( 'ul' )[0];

Or target an ID value of "site-navigation" and change line 38 in header.php to:

<nav id="site-navigation" class="main-navigation" role="navigation">

We probably don't need both classes there, it was a hold-out from the JS changing the class to "main-small-navigation".

[lancewillett]

No jQuery, show and hide menu with click handler

[lancewillett]

Better JS menu hide/show with support for wp_page_menu markup

[lancewillett]

This ticket is getting crazy. :)

I think our best option for v 1.0 launch is to use this non-jQuery technique without the CSS transition. It works in all browsers and mobile devices, and gets the job done without loading jQuery.

The CSS transitions would be a nice enhancement, but could be added post-launch without adverse effect on anyone.

[lancewillett]

@obenland Note for once we have this part done: check IE9, the menu isn't showing up at all. Best to debug after we finalize the menu markup and CSS, though.

[lancewillett]

Same as .5 with minor JS file cleanup

[azaozz]

For the JS addClass and removeClass:

function removeClass(element, name) {
  element.className = element.className.replace( new RegExp('\\b'+name+'\\b', 'g'), '')
    .replace(/^ | $/, '').replace(/ +/g, ' '); // trim spaces
}

function addClass(element, name) {
  removeClass(element, name); // avoid duplicates
  element.className += ' ' + name;
}

Perhaps consider making the menu fully accessible, instead of using display: none and display: block, much better to use top: -10000em to hide from the viewport but not from screen readers and top: 0 to show (the submenus are already position: absolute anyway). Then use :focus as well as :hover to show them.

This makes the sub-menus fully accessible when "tabbing" too. Changed that on the admin menu and it works a lot better now.

[lancewillett]

Simpler regex check for class value, add back in the empty menu check, can remove once core fix is in

[Jayjdk]

Is matchMedia.js script in html.js still needed after [21554]?

[Jayjdk]

Removes the matchmedia script and adds patch from #21671

[lancewillett]

Replying to Jayjdk:

Is matchMedia.js script in html.js still needed after [21554]?

Good point.

[lancewillett]

In [21603]:

Twenty Twelve: remove matchMedia JS polyfill now that older browsers are getting mobile-first navigation styles. Props Jayjdk, see #21562.

[lancewillett]

Note from Koop from a JS code review in IRC yesterday, in navigation.js we should check the existence of the button and menu elements before operating on them (exit the function early if null or empty, etc).

[lancewillett]

Adds JS check for undefined menu and button elements

[lancewillett]

Replying to azaozz:

For the JS addClass and removeClass:
function removeClass(element, name) {
function addClass(element, name) {

I think that's probably overkill. Might as well just go back to using jQuery methods at that point. :)

Perhaps consider making the menu fully accessible, instead of using display: none and display: block, much better to use top: -10000em to hide from the viewport but not from screen readers and top: 0 to show (the submenus are already position: absolute anyway). Then use :focus as well as :hover to show them.

I like this—but in the case of the small mobile menu the hover and focus pseudo classes don't apply since this display mode is for touch devices primarily. The submenus are not positioned in this display (they are in > 600 pixels only).

In the "large" display of the menu the top-level is always visible.

[lancewillett]

For new topic of small navigation menu layout, please see #21678 so we can keep this ticket focused on the JS hide/show behavior.

[lancewillett]

In [21611]:

Twenty Twelve: rework navigation to remove need for jQuery and support wp_page_menu markup better. Fixes #21562.

[azaozz]

Replying to lancewillett:

...

function removeClass(element, name)
function addClass(element, name)

I think that's probably overkill. Might as well just go back to using jQuery methods at that point. :)

Right. These are generic replacements that would work well anywhere and for any class. When this is only needed for a specific class, it can be hard-coded and the code will be shorter.