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.

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

[UX] Provide a hamburger menu (or option) by default #2369

Closed
laryn opened this issue Nov 29, 2016 · 50 comments
Closed

[UX] Provide a hamburger menu (or option) by default #2369

laryn opened this issue Nov 29, 2016 · 50 comments

Comments

@laryn
Copy link
Contributor

laryn commented Nov 29, 2016

Is there a way to get the default menus to turn into a "☰ Menu" link that opens to the full menu, instead of the stacked version that is used by default? With any more than a few menu items it currently takes the whole screen on a phone and it would be nice to have it collapsed to start with.

Pulling a few of the responses from Gitter:

@klonos : Yes, it is possible. Take a look here: http://www.smartmenus.org/docs#menu-toggle-button

@herbdool : it'll take editing the tpl at least to get the hamburger to work since it's picky about where the toggle is in relation to the menu. ...
I got the hamburger working. Use MYTHEME_menu_tree__main_menu() to inject the Smart menu html for the toggle button, then edit and add the css and js to your theme. You need to change #main-menu to .menu in the css, and make sure you have the right selector in the js.

function MYTHEME_menu_tree__main_menu($variables) {
  $return = '';
  if ($variables['depth'] == 0) {
    $return = '<!-- Mobile menu toggle button (hamburger/x icon) --><input id="main-menu-state" type="checkbox" /><label class="main-menu-btn" for="main-menu-state">  <span class="main-menu-btn-icon"></span> Toggle main menu visibility</label>';
  }
  $return .= '<ul ' . backdrop_attributes($variables['attributes']) . '>' . $variables['tree'] . '</ul>';
  return $return;
}

it puts the hamburger below the logo which may or may not work for you. I haven't tested to see if it can work without needing to be a sibling of the menu. If so then the hamburger html could be put anywhere

@klonos : I would absolutely love it if there was a simple way in the menu block settings allowing the user to select the hamburger as an alternative option for mobile (instead of the default responsive style we have now). I have come across some websites that use the hamburger as their default navigation regardless of screen size, so I believe that the best way to handle this would be if we also allowed it to be selected as an individual menu style option in menu block settings.

@herbdool : The hamburger still uses the responsive menu that's there but just provides a toggle for it.

@olafgrabienski : If you file that hamburger menu issue, I guess these are good reference points: #2107#issue-173965498 (we want a hamburger) and #2107 (comment) (why isn't it there at that moment / how to implement it in future).


PR from @herbdool backdrop/backdrop#1686
PR from @jenlampton backdrop/backdrop#1708
PR from @njbooher backdrop/backdrop#1714
PR from @jenlampton backdrop/backdrop#1715

@laryn laryn changed the title Provide a hamburger menu (or option) by default UX: Provide a hamburger menu (or option) by default Nov 29, 2016
@laryn laryn changed the title UX: Provide a hamburger menu (or option) by default [UX] Provide a hamburger menu (or option) by default Nov 29, 2016
@olafgrabienski
Copy link

Thanks for making the feature request. A hamburger menu was one of the designated features of issue #2107 (Provide responsive drop-down menus out of the box):

Add necessary JS to turn the menu into a hamburger (and the word "Menu") when it runs out of room.

To get the responsive drop-down on time in the 1.5 release, it was committed without the hamburger menu. @quicksketch said then in #2107 (comment):

My thought is to have Basis implement a hamburger but exclude that from Bartik. Basis would likely do this through a combination of JS, theme_menu_tree(), and additional CSS.

Is there anybody who wants to make a PR based on @quicksketch's proposal and/or @herbdool's approach mentioned above?

@wesruv
Copy link
Member

wesruv commented Nov 29, 2016

I have some code for this that I implemented on another site, I'll try to get a PR together soon, playing catch up 😛

@olafgrabienski olafgrabienski added this to the 1.5.3 milestone Dec 1, 2016
@laryn
Copy link
Contributor Author

laryn commented Dec 7, 2016

@wesruv Was it similar to @herbdool's approach? If so, is it Basis-specific? If you are overwhelmed I may try to replicate this and submit. If you have it handy, obviously that's better for me. :)

@quicksketch quicksketch modified the milestones: 1.6.0, 1.5.3 Dec 8, 2016
@quicksketch
Copy link
Member

As this is a feature, not a bug, I'm moving this to 1.6.0.

@olafgrabienski
Copy link

Yes, it's a feature, but it was one of the designated features of the "Provide responsive drop-down menus out of the box" issue, and in my opinion it's important to provide the missing feature, no matter if it's a feature request or a bug report.

@jenlampton
Copy link
Member

jenlampton commented Dec 15, 2016

This was the first feature-request I had on my D7 module for smartmenus too here and there is some recommended code at http://www.smartmenus.org/docs/#menu-toggle-button that makes it pretty easy.

@wesruv amd @laryn you might want to review the commit there to see how that was done, it may helpful. The difference there is that I added a block/menu setting for if the hamburger was wanted, and here it sounds like we'll always be adding it in the theme. I think I like the setting approach better, but I'll leave it up to you guys to decide.

@herbdool
Copy link

I've taken the approach of adding a setting to the Basis theme. It'll only include the hamburger if the primary nav is present.

@jenlampton
Copy link
Member

if the primary nav is present.

What does this mean? If a menu block is present? If the menu block is showing the 'Primary navigation' menu?

@herbdool
Copy link

Out of the box Basis has a Primary Navigation block enabled. This provides an option for a hamburger on that menu alone. It's main_menu in the system.

@olafgrabienski
Copy link

olafgrabienski commented Dec 23, 2016

Thanks @herbdool for the PR. Tested it, works well!

I like that there is a setting but as theme setting, it seems a bit far away. Without knowing what it would mean technically, I'd prefer to have the setting in menu blocks.

I guess, we could need some more feedback, tagged the issue as such.

@jenlampton
Copy link
Member

My vote is for a block setting. That way it will work for all themes.

@laryn
Copy link
Contributor Author

laryn commented Dec 23, 2016

I agree, a block setting seems best from a user standpoint. I don't know how much that complicates the PR or if it means a completely different approach.

@herbdool
Copy link

Makes sense to me. It can be bundled with the Primary Navigation block which also has the option for the dropdown. I'll look into it.

@klonos
Copy link
Member

klonos commented Dec 24, 2016

Yep, the menu block is where I'd expect a setting for that to exist. Perhaps as a "Render as a hamburger menu" checkbox shown via #states if "Dropdown menu" is selected as the menu style. When that checkbox is enabled, there would be another set of radio options:

( ) Always
( ) Below a specific breakpoint: [ field to set breakpoint (in px/em??) ]

@klonos
Copy link
Member

klonos commented Dec 24, 2016

...btw, it would be great if we had https://www.drupal.org/project/breakpoints in core so that themes and modules (like the admin_bar.module) can use them.

@klonos
Copy link
Member

klonos commented Dec 24, 2016

...actually not themes. In Backdrop layouts could use the breakpoints. Right?

@herbdool
Copy link

I've updated my PR. That was a bit harder to figure out than I thought. I don't know if I've got a good approach but it seems to be the only way I've gotten it to work.

There's a checkbox when you configure the Primary Navigation only and it allows you to display the menu toggle. I've created a template block--system--main-menu.tpl.php where the button is added.

@herbdool
Copy link

@klonos good suggestions re breakpoint, but I see those as new feature requests that can be added later.

@jenlampton
Copy link
Member

I've created a template block--system--main-menu.tpl.php where the button is added.

Hm, that doesn't sound right. The markup should be in the menu output itself, not in the output for the block around the menu. Yes, this sounds tricky... Hm...

@jenlampton
Copy link
Member

jenlampton commented Jan 1, 2017

How important is it that the hamburger work for any menu?

It's very important that we don't make assumptions about which menu people are going to be using as their main menu. However, I don't think it's super important that it work for more than one menu at a time. It looks like the changes you made are going to cover those bases @herbdool, nice work!

@wesruv sounds like the ball is in your court? Looks like we're really close! :)

@jenlampton jenlampton self-assigned this Jan 2, 2017
@jenlampton
Copy link
Member

I'm going to take a stab at this one since I just added/fixed a similar feature on https://www.drupal.org/project/smartmenus :)

@jenlampton
Copy link
Member

New PR is up at backdrop/backdrop#1708

I did some reworking of the CSS provided by @wesruv because the selectors weren't inline with the way we do CSS in the rest of core. I also added back the JS effect for sliding the menu open/closed since I couldn't determine if the CSS solution (using VH) would work on all browsers, and the JS is a proven solution. ( I basically put it back to the recommended CSS from the smartmenus library, but we can iterate and improve after Jan 1st if necessary.) I also cleaned up the markup to be easier for devs to read, and some of the PHP too, since I was in there anyway.

This is ready for user-testing if we have any takers :)

@jenlampton jenlampton removed their assignment Jan 2, 2017
@olafgrabienski
Copy link

Had a quick look at the new PR: The hamburger button and the block setting (if to show the hamburger button on small screens) worked fine.

@herbdool
Copy link

herbdool commented Jan 3, 2017

@jenlampton I've put some comments on the PR

@Dinornis
Copy link

Dinornis commented Jan 3, 2017

Which PR needs testing?
1808 does not work and 1712 runs the install.

@jenlampton
Copy link
Member

backdrop/backdrop#1712 needs testing.

@quicksketch
Copy link
Member

quicksketch commented Jan 3, 2017

I closed the previous PR as the sandbox functionality seemed broken. Latest PR is now at backdrop/backdrop#1714.

Some feedback:

  • I'm not a big fan of the random hashes getting assigned to the menu IDs. Especially with these hashes getting assigned into config files. Could we just use backdrop_html_id() to ensure unique identifiers in the same way that Form API does?
  • The word "Menu" seems to be invisible on mobile, for accessibility and explanation, I think it would be best if the default CSS included the display of this text:
    screenshot from 2017-01-02 20-37-21
  • As a potential follow-up, it's very strange now with the "secondary" links being huge and the primary links stashed away in a hamburger, as you can see from the above screenshot. We need better styling in Basis for the header menu links after this change.

@jenlampton
Copy link
Member

Could we just use backdrop_html_id() to ensure unique identifiers in the same way that Form API does?

Can you think of an example of that we can copy?

The word "Menu" seems to be invisible on mobile

Hm, I missed that. I agree, we can get it back.

As a potential follow-up...

Yes, let's work on that tomorrow :)

@jenlampton jenlampton self-assigned this Jan 3, 2017
@quicksketch
Copy link
Member

backdrop_html_id() just automatically appends --1, --2, etc if the same ID is used multiple times on the same page. So we just pass in the HTML ID as a variable in each theme function, e.g.

$variables['id'] = backdrop_html_id('menu-toggle');
theme('menu_toggle', $variables);

Then we just let backdrop_html_id() deal with giving a unique identifier if there are multiple toggles on the page.

@jenlampton
Copy link
Member

okay, I think I got it:

      $id = backdrop_html_id('menu-toggle-state');
      $data['content']['#wrapper_attributes']['data-menu-toggle-id'] = $id;
      $data['content']['#prefix'] = theme('menu_toggle', array('enabled' => $config['toggle'], 'id' => $id, 'text' => t('Menu')));

@quicksketch
Copy link
Member

Yay, new backdrop_html_id() approach works great and saves us from those hashes ending up in the config files. I made a few last tweaks to the CSS and docblocks. Merged backdrop/backdrop#1715 into 1.x for 1.6.0. Woot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants