Default state of toolbar should show menu tray in non-narrow viewports

Bojhan, if you still think we should do this, then we can do this tomorrow at DC Ghent 2014.

One problem with this proposal is that we must NOT do this on narrow viewports, because there the vertical tray would be used. Which means we'd lose a lot of screen real estate. So it'd have to be a "smart default".

Cool — clarifying title then :)

Whoever takes this on: #2103247: Clicking menu links in the administration menu tray should close the admin menu tray, while in a narrow viewport where the toolbar is positioned on top of the content is somewhat related, please read that for inspiration on how to do this.

I'm not really sure this is a good attempt to fix the problem. With this patch the first menu tab wil be 'opened' by default if no tab was explicitly clicked before and only when the orientation of the tray is horizontal. I also stumbled upon some issues here:

• I can only show the tray by default when the toolbar is positioned horizontally, on tablets and small screens you just don't have enough space on your screen. OTOH, the horizontal tray is less obtrusive in the UI
• By enabling the default tab item when no active tab is selected, I can see the 'Manage' tray by default (when I view my profile for example). I think this may be a usability issue.
• I don't like the way I hard code the tab ID in this patch. Any suggestions?

Fixing inconsistency in comment.

I don't like the way I hard code the tab ID in this patch. Any suggestions?

That's indeed the biggest problem in the patch.

We basically want to always get the first tab that is not the "Home" tab, which is a special snowflake, and if it exists, it's always going to be first. So we need a clean way of detecting it: we want a class to be set on that toolbar tab, just like there already is a class being set on the "shortcut" and "tour" toolbar tabs. Once we have that, we can use this selector and use the first matching element, and the ID for that element is the tab we want to activate:

.toolbar-bar .toolbar-tab:not(.home-toolbar-tab)

To add that class, do this:

diff --git a/core/modules/toolbar/toolbar.module b/core/modules/toolbar/toolbar.module
index 9a7914b..f95a890 100644
--- a/core/modules/toolbar/toolbar.module
+++ b/core/modules/toolbar/toolbar.module
@@ -133,7 +133,7 @@ function toolbar_toolbar() {
       ),
     ),
     '#wrapper_attributes' => array(
-      'class' => array('hidden'),
+      'class' => array('hidden', 'home-toolbar-tab'),
     ),
     '#attached' => array(
       'library' => array(

Thanks for the pointers, Wim. In this patch I added a class to the 'back to site' tab in the toolbar and now the first element is selected correctly.

1. +++ b/core/modules/toolbar/js/toolbar.js
   @@ -112,6 +112,14 @@
   +        // defined then show the tray of the first toolbar tab by default.
   

   should mention that we exclude the 'home' toolbar tab.

2. +++ b/core/modules/toolbar/js/toolbar.js
   @@ -112,6 +112,14 @@
   +        if (Drupal.toolbar.models.toolbarModel.get('orientation') == 'horizontal' && Drupal.toolbar.models.toolbarModel.get('activeTab') === null){
   

   == 'horizontal'
   Please use strict equality.

3. The hunk you added in the JS comes before the code that sets up broadcasting of model changes. Hence it's going to be impossible for modules to be notified of the active tray. Please move your new hunk in the JS to be the very last piece of code to run.

• Mentioning that the home toolbar item should be excluded from default tab selection.
• Using strict equasion for 'horizontal'
• Moved hunk of code to the bottom
• Changed the way the first element of the toolbar-tab item is accessed (getter instead of index of array)

Only one nitpick remains, then this is RTBC — thanks for your work! :) I manually tested this, and it works perfectly :)

+++ b/core/modules/toolbar/js/toolbar.js
@@ -123,6 +123,15 @@
+        // If the toolbar's orientation is horizontal and no active tab is
+        // defined then show the tray of the first toolbar tab by default
+        // (but not the first 'Home' toolbar tab).

80 cols rule.

80 cols rule.

I tried to find it but I guess I'm looking over something. The longest line in the comments I made is 75 columns wide. However, there are some comments in toolbar.js(that I didn't write) that violate this rule. Fixed in patch.

What line is it exactly in?

We don't only say that the *maximum* length is 80 cols, we also say that *if* a word on a following line fits on the previous line without exceeding 80 columns, that it should be on the previous line. *That* is what you violated.

Generally, we don't fix unrelated coding standards violations like you did in #15, because it makes reviews too difficult. This patch is small enough to do that though, so it's fine.

If you can just fix #14 also with the extra info I just gave you, this will be RTBC :)

Fixed #14 (80 columns rule)

Awesome, thank you!

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 232c10d and pushed to 8.0.x. Thanks!