WordPress.org

Make WordPress Core

Opened 21 months ago

Closed 20 months ago

Last modified 20 months ago

#21562 closed defect (bug) (fixed)

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

Reported by: cfinke Owned by: lancewillett
Milestone: 3.5 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (15)

21562.patch (546 bytes) - added by cfinke 21 months ago.
Hides the 'Show navigation' button if there are no navigation menu items.
21562.2.diff (4.0 KB) - added by obenland 21 months ago.
21562.css-menu-switch.diff (4.7 KB) - added by obenland 21 months ago.
21562.css-menu-switch.2.diff (8.8 KB) - added by obenland 20 months ago.
21562.3.diff (9.5 KB) - added by lancewillett 20 months ago.
Added menu-toggle class in header.php, minor cleanup
Screenshot_2012-08-20-15-41-02.png (63.2 KB) - added by nacin 20 months ago.
the centering and tons of padding seem like an ineffective use of space
Screenshot_2012-08-20-15-31-33.png (336.0 KB) - added by nacin 20 months ago.
the headline and image are a hit close together here
21562.no-jQuery.diff (2.7 KB) - added by obenland 20 months ago.
21562.4.diff (2.5 KB) - added by lancewillett 20 months ago.
No jQuery, show and hide menu with click handler
21562.5.diff (3.0 KB) - added by lancewillett 20 months ago.
Better JS menu hide/show with support for wp_page_menu markup
21562.6.diff (3.0 KB) - added by lancewillett 20 months ago.
Same as .5 with minor JS file cleanup
21562.7.diff (4.6 KB) - added by obenland 20 months ago.
21562.8.diff (4.9 KB) - added by lancewillett 20 months ago.
Simpler regex check for class value, add back in the empty menu check, can remove once core fix is in
21562.matchmedia.diff (3.4 KB) - added by Jayjdk 20 months ago.
Removes the matchmedia script and adds patch from #21671
21562.9.diff (5.1 KB) - added by lancewillett 20 months ago.
Adds JS check for undefined menu and button elements

Download all attachments as: .zip

Change History (58)

cfinke21 months ago

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

comment:1 cfinke21 months ago

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

comment:2 cfinke21 months ago

  • Keywords has-patch added

comment:3 obenland21 months ago

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

comment:4 obenland21 months ago

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?

obenland21 months ago

comment:5 follow-up: obenland21 months ago

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.

comment:6 in reply to: ↑ 5 lancewillett21 months ago

  • Milestone changed from Awaiting Review to 3.5
  • Severity changed from minor to normal

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.

comment:7 lancewillett21 months ago

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.

comment:8 lancewillett21 months ago

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

comment:9 lancewillett21 months ago

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

comment:10 lancewillett21 months ago

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.

comment:11 lancewillett21 months ago

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

Last edited 21 months ago by lancewillett (previous) (diff)

comment:12 obenland21 months ago

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

comment:13 follow-up: obenland20 months ago

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.

comment:14 in reply to: ↑ 13 lancewillett20 months ago

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.

comment:15 lancewillett20 months ago

  • Summary changed from Twenty Twelve: Hide the "Show navigation" button when there is no navigation to show. to Twenty Twelve: improve navigation to be mobile-first and not rely on JS for hiding and showing

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.

lancewillett20 months ago

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

comment:16 lancewillett20 months ago

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.

comment:17 lancewillett20 months ago

  • Keywords has-patch removed

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

comment:18 lancewillett20 months ago

In [21557]:

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

nacin20 months ago

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

nacin20 months ago

the headline and image are a hit close together here

comment:19 lancewillett20 months ago

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

comment:20 lancewillett20 months ago

In [21564]:

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

comment:21 lancewillett20 months ago

In [21573]:

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

comment:22 follow-up: spencerdahl20 months ago

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

comment:23 in reply to: ↑ 22 ; follow-up: lancewillett20 months ago

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

comment:24 in reply to: ↑ 23 obenland20 months ago

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

obenland20 months ago

comment:25 follow-ups: obenland20 months ago

@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??

comment:26 in reply to: ↑ 25 ; follow-up: lancewillett20 months ago

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

comment:27 in reply to: ↑ 26 lancewillett20 months ago

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.

comment:28 follow-up: obenland20 months ago

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

comment:29 in reply to: ↑ 25 ; follow-up: Jayjdk20 months ago

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

comment:30 in reply to: ↑ 29 obenland20 months ago

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.

comment:31 in reply to: ↑ 28 lancewillett20 months ago

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

lancewillett20 months ago

No jQuery, show and hide menu with click handler

lancewillett20 months ago

Better JS menu hide/show with support for wp_page_menu markup

comment:32 lancewillett20 months ago

  • Keywords has-patch added

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.

comment:33 lancewillett20 months ago

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

lancewillett20 months ago

Same as .5 with minor JS file cleanup

obenland20 months ago

comment:34 follow-up: azaozz20 months ago

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.

lancewillett20 months ago

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

comment:35 follow-up: Jayjdk20 months ago

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

Jayjdk20 months ago

Removes the matchmedia script and adds patch from #21671

comment:36 in reply to: ↑ 35 lancewillett20 months ago

Replying to Jayjdk:

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

Good point.

comment:37 lancewillett20 months ago

In [21603]:

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

comment:38 lancewillett20 months ago

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

lancewillett20 months ago

Adds JS check for undefined menu and button elements

comment:39 in reply to: ↑ 34 ; follow-up: lancewillett20 months ago

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.

comment:40 lancewillett20 months ago

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

comment:41 lancewillett20 months ago

  • Summary changed from Twenty Twelve: improve navigation to be mobile-first and not rely on JS for hiding and showing to Twenty Twelve: improve navigation to be mobile-first and not rely on jQuery for hiding and showing

comment:42 lancewillett20 months ago

  • Owner set to lancewillett
  • Resolution set to fixed
  • Status changed from new to closed

In [21611]:

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

comment:43 in reply to: ↑ 39 azaozz20 months ago

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.

Note: See TracTickets for help on using tickets.