Initial D7UX admin header

Split these into two issues, please.

subscribing

1) I do not think this needs to be a theme at all. We can do without, and by doing so, achieve a more consistent behavior.

2) I agree with chx, this needs to be split.

2b) The fieldset markup, styling, and behavior change is completely unrelated to the topic.

3) Recent patches to Admin module do not seem to be contained. Mostly, I'm worried about this header being in conflict with admin_menu.

Patch applies cleanly - looks awesome!!!

Suppose this shoud be sepearate issues for theme and module

PS: Screens for FF3-win

An enthusiastic subscribe!! :D I agree with chx that this'll be easier to get in if we de-couple the admin theme from the header bar. Could we make this issue one just about the header bar?

Also, whoever sets up a demo site with this patch applied for testing will be my very, very best friend.

for anyone wanting to test this w/out applying the patch:

Host: http://usability1.atendesigngroup.com/
username: admin
password: test

@ beeradb - Thank you! Haven't had a lot of d.o time lately, so having this is quite awesome.

Just a note: One of the things that content managers (who this admin section is for) do most often is create new content. When you click on that option, all the sudden you're booted out of the admin menu chain of events that you were just in, which is kind of jarring.

I would like the Admin menu to be aware of the "Add new content" page and still show the section with "Content" selected, until I'm finished managing/editing/creating new content.

[Edit: Also, lol @ the 'Roles' drama icon. I like it. Kinda tongue-in-cheek. The only icon I'm not that excited about is the permissions icon. It makes me think "movies" more than "admission." Maybe that's just the American movie-addict in me speaking?

Speaking of icons, is there any particular reason why most of the user settings icons are in shades of grey?]

IMHO, Admin menu looks a lot like the redesigned toolbar of MS Office 2008. Now, how you feel about that is a whole different issue, but it kindof takes a lot of space.

@ irakli - the thing is, a lot of the 'content editors' who would be candidates for using this admin system like big, gooey buttons. They aren't as happy as developers are about seeing a bunch of small lists of text options, like we see in administration menu. I love the admin menu module, and use it everywhere, but rarely deploy it for end users/content managers on Drupal sites, because it's more hassle than it's worth training them on all the sections. I simply set up a nice little menu for them on the left with big text. It takes up space, but makes it easier for them to use the site.

Added tag.

http://admin.drupal4hu.com user d7ux password webchickR0cks! -- this is uid1 so be gentle, ok?

Here is my first review, I will post more when it's not 1:32AM: holy shit!

Wow! The toolbar is so awesome and useful!

[I think the slate theme has a lot to improve, which is anyways moving into another issue. There's a bit too much flatness in the design, reminds of development seed designs. My opinion. :) ]

What does the arrow in "Text format" icon imply? Or how does it imply that it's text format / filters / html, etc. stuff?

Maybe there could be a tunnel / cylinder in between a raw item and a product.

yet another comment.

When we re-click a top level menu item, the submenu does not close. Is it intentionally so? I really hate to reach the (X) link on the right.

yet another ................

when clicking (x) close link, the "active" class on the top level menu item should be removed.

I guess this new "Admin tools" region was introduced by this patch itself, but why's that we can't move around the admin header?

Sorry for another comment, quite excited, even if short term excitement, I'm glad I'm following issues again. lol

Still too big a scope for one patch. And while looking sexy etc. it doesn't even start to adress the underlying ux issues we should have a direction on first before we add the pretty.

It's easy to get excited about this part because it does look good etc. but we should start building bottom up.
First step should be to introduce the region this header can live in. This work has been started here: #468534: Add a region at the top of the page above the header region.

Please, join forces there. please.

oops

+1 Yoroy - it looks pretty, however it does move in quite different directions to what we've proposed for D7UX (esp. the contextual icons in the second level), and it doesn't address most of the key objectives of D7UX. Except for a familiar visual styling, I don't see much of our work in this header.

FYI: my take on the key objectives for D7UX as copied from an earlier IRC conversation:

1. improving a bunch of interfaces that sorely require usability attention, primarily because they are overly complex
2. improving the system wide design of Drupal so that it is a more coherantly usable and pleasant experience
3. trying to find a way to map the way that drupal works to the way that human beings work so that people who are new to drupal don't have the horrendous experience that they have now
4. giving drupal developers an admin interface that they can proudly and happily give to their clients/etc. to administer the site without having to spend days training them how to use it and supporting them ongoing

This header takes a step towards that pride I'm referring to in step 4, but not much else.

A question in ref: to yhahns comment above - what's so bad about an admin theme?

As mentioned above, lets first explore the reasons for this admin header making ground for choises. Instead of creating the style first, as astehticly it may appear more usable, it doesn't solve the actual usability problems.

What we need is: (i) design improvements + (ii) usability improvements.

This patch is mostly a design improvement and not a usability improvement -- it gives us some of (i) but very little of (ii). It is "bling", and great bling at that. If we don't follow up with usability improvements, it is "lipstick on a pick" (see my DrupalCon keynotes).

However, it seems like a good interim step as long we follow-up with some restructuring to make it more usable too. It is good to establish the header pattern, and to get that code to work nice on all browsers as well as degrade properly. Because the proposed header will need work to make it more usable, we shouldn't obsess about the icons at this stage. That could and probably should be done after usability improvements made it in.

Once the design (really, the "header pattern") is committed to core, it probably becomes easier to focus on the usability which is what the real challenge is. There is a risk in that as well, as design and usability often go hand in hand.

ok. a couple of thoughts:

IMHO design changes that don't enhance usability are not design improvements, they are exactly as Dries said, 'lipstick on a pig'. The separation of design and usability is an artificial separation - usability happens via design.

I really don't want to promote the addition of 'design' that does not include usability enhancements and potentially, puts good usability at risk.

having said that - I understand (and am excited by) the eagerness to move forward on the header pattern.

If we say that the header Information Architecture maps exactly to current core Information Architecture AND the icons are removed from the second level of navigation then I can see this as a repositioning and 're-colouring' of the admin navigation and I can live with that as a first step to the d7ux header.

I might be missing something, but has the create content link been lost or completely buried in this design? We'll need that somewhere.

Rationale re: icon removal - it is really important that if we are going to use icons in D7, we have a really strong strategy and guidelines as to how they are used. The usage shown in this implementation is problematic - esp. re scalability. I think we should take time to consider this v carefully (and I don't think they're critical to achieving this first step towards the header pattern at any rate)

subscribing.

Selfishly Subscribing.

If you only read one comment on this list, read #29 by Dries. If you only read two comments on this list, read #29 by Dries and then #27 by leisa.

Here's a quick review, more later:

Initial review of the UI based on chx's demo site, I've only very quickly skimmed through the code but will try to have a proper look later.

At the moment, I navigate Drupal in two ways. If I know where I'm going, I use my browser address bar. If I don't, I go to the next closest place (usually admin/settings, admin/build etc.) and find the link from there. (let's assume I'm not looking in hook_menu() for now).

The header as it stands, gives you a top-level navigation for 'content', 'site building' etc. - when you click on those, you get second level navigation links (to admin/content/node or admin/settings/site-information etc.). If I click on the top-level item when it's child items are open, I'd expect one of two things to happen:

1. I get taken to admin/content or admin/site-building
2. It closes the header back up again.
Neither of these are the case - those links do nothing if you click them a second time (instead there's an x to close the expanded region).

There's actually no way to get to those top level sections (admin/content etc.) from this header. And not all links are shown in that second level navigation admin_menu style, only ones which are specified in hook_menu().

So if I need to get to an admin screen which isn't available from the header bar, I don't have any options but to go to /admin manually and look through. Or to click a link I don't want, then go up via the breadcrumb to where I actually wanted to get to. For me, that means massively increasing the number of clicks I'd need to make to administer any Drupal site and would be a regression in terms of actual user experience.

For new users, assuming the admin menu is the first navigation they see, it means there's absolutely no way to get an overview of any Drupal admnistrative section from the admin header - you can only get there by accident, instead you get links which are (currently) hard-coded and take you straight into the depths of a specific configuration setting. While /admin and admin/settings are a bit scary apparently - in usability testing we saw many users scanning for familiar keywords, and often finding what they wanted getting orientated - quick shortcuts from the top of the page works for this even with our existing crappy IA, hiding those sections don't. People who actually want a lot of control over their site (which does account for a decent number of Drupal users), I think would find the subset of links and no obvious breakout restrictive.

If you look at http://www.d7ux.org/header/ - you get far fewer links - a list of your main admin silos which takes you straight to them with no extra clicks (which I'd probably use when firefox history and memory fails me), then user/role configurable shortcuts (which I'm less likely to use but since there's a hide/show they wouldn't bother me). By going direct to the silos, I can immediately get an overview of what's possible. And it's no extra clicks to get where I want (because I'd still have to click the second level nav on the demo site to actually get anywhere).
This logic applies even more so if modules/configuration ends up using the harmonica stuff yoroy is working on - since we'll have far fewer configuration screens which just let you change three variables and nothing else.

I also agree this isn't the issue to be introducing an icon set into core. Discussions over icons are going to needlessly delay the patch, and once icons are in, we all know it can take years to change them. Since the icons are just images and css rules, it should be easy to split those into a separate patch - which would make the actual header changes here a lot easier to review.

subscribe

Subscribing. Love the icons! (Agreed that adding them to core is best left to another issue, though - this one is gigantic enough as it is.)

Some comments on the patch, both at the code and usability level:

• The "contextual admin links" (a.k.a. the edit/delete links that appear when hovering over a node) do not seem related to this issue; please see #473268: D7UX: Put edit links on everything where a patch is already in progress. There, we are trying to implement them as something more generic that is tied to the page region, rather than via node-specific code (thereby allowing the links to appear for nodes/blocks/views/etc all at once in a consistent way). That patch is stalled on a couple issues right now, and I think your code would help a lot - one thing, for example, is that we haven't really even started trying to style the links yet, but the way you've implemented it here is similar to what has been discussed. Please consider moving this part of the code to #473268: D7UX: Put edit links on everything and seeing if it can be integrated with the efforts there.
• Although there are some really nice things about having the second-level links in the menu be context-dependent, one potential huge problem is that it gives the impression that everything you might want to do on your Drupal site can be reached through that menu, even though it can't (note: I started writing this before seeing @catch's review above, but I think it's a similar point). So if for example I am looking to configure my site's taxonomy, I can click around the header all day and never find a link to it; I need to figure out that the only way to get there is via some other click-path that does not involve the header at all. To me, that seems very unintuitive. It's a potential problem with Mark and Leisa's design as well, but less so because there the header region contains a few links that always stay the same. I think we're going to need to come up with a solution to this problem no matter what, but one simple suggestion in the context of the current patch might involve putting some kind of "More" link in the header region...?
• I like the 'options' array for indicating whether something should be in the header, but also making this configurable via the user interface seems like it would be a requirement for getting this in core; Drupal already lets you configure all the other menus, so why not this one? I guess I always envisioned this header menu as being something that has its own separate entry at admin/build/menu (just like e.g. the Navigation menu and other menus which core treats specially) and therefore you could add/remove links to this menu just like any other, while still allowing modules to provide sensible defaults at the code level. I haven't looked deeply enough into this part of the code, but it seems like implementing it that way might allow some code in this patch to be removed. I'd rather see Drupal's underlying menu system flexible enough to accommodate this kind of menu, rather than adding a lot of "menu-like" code in a separate, optional module.
• Minor point -- well, not so minor, but easy to fix :) -- The logout link does not currently work; in D7 it needs to point to user/logout rather than logout.

@leisareichelt:

A question in ref: to yhahns comment above - what's so bad about an admin theme?

As I understood it, those comments were more about whether an admin theme should be part of this patch, not whether Drupal should have one at all?

More specifically, it's better if these usability improvements (such as the header, etc) can work with any theme, rather than being tied to a specific one. I think that may have also been part/all of the rationale for marking this issue postponed in favor of #468534: Add a region at the top of the page above the header region. -- to make sure Drupal core provides a generic configurable region that can be used for this kind of header (?).

ah, thanks David, that makes sense.

Although there are some really nice things about having the second-level links in the menu be context-dependent, one potential huge problem is that it gives the impression that everything you might want to do on your Drupal site can be reached through that menu, even though it can't ... It's a potential problem with Mark and Leisa's design as well, but less so because their the header region contains a few links that always stay the same. I think we're going to need to come up with a solution to this problem no matter what, but one simple suggestion in the context of the current patch might involve putting some kind of "More" link in the header region...?

this is one of the key reasons that we've proposed the icon layer of the navigation to be like a shortcut doc for common tasks rather than the second level of navigation - it's not scalable (and that's not even to mention trying to come up with an icon for everything that might go in there that is actually meaningful).

our current proposed approach is to, as you say, have fewer items in the global navigation so that the number of places you have to click around to find things is limited, and then to design 'landing' or 'index' pages that quickly signpost you to what you want to get to.

This coupled with another Information Architecture strategy we are in the process of working through (having gotten to it originally via discussion in IRC with Catch & Yoroy and others) is that we separate out the types of functions that people access on a day to day basis from those that are accessed irregularly. For example, in our proposed IA, the Content section will be all about creating and editing content and nothing to do with how it is structured, configured etc. All of the 'settings' type functions would be moved to a section of their own (currently called Modules & Config but I think that may need to shift) with cross linking where appropriate.

We're still testing against a whole lot of scenarios (and welcome more) but so far it seems to scale quite nicely and support wayfinding in Drupal for both novices and experienced users.

(apologies if that is a complete tangent, but wanted to allay any concerns that we weren't thinking hard about the scalability aspects of our proposed approach).

1) Finally, there are some real issues people can discuss and track. Thanks for using existing and known-to-be-working project flows.

2) As already stated in #2, I don't think that anything from what is proposed here will work reliably as theme, template variable, and/or theme region block. See also #468534-49: Add a region at the top of the page above the header region..

3) If at all, I would suggest to develop Admin module in contrib and move it afterwards (if at all). Provides niceties like issue subscriptions, overview of things to tackle, fast evolvement and development. Particular issues can be moved into core's queue when needed. Ensures that the entire functionality is kept optional.

4) In the end, all of my concerns about this duplication of efforts, work, and issues become reality. That really annoys me. Just keep going re-inventing the wheel. It's a major waste of time.

I am +1 removing the icons from this patch. There is another issue already open that looks at GPL icons for Drupal. I do think it makes sense to remove the icons from this one and work on "global" icons in #13911: Drupal GPL icons.

@yhahn: Wow that's a lot of work to roll this patch. Congrats on pulling it all together and putting up something that we can all rip to shreds. ;) The clicky open thing is cool. But I'm so used to admin_menu (http://drupal.org/project/admin_menu) and root candy admin theme (http://drupal.org/project/rootcandy) that I'm not feeling a "wow" factor...yet.

Argh. Cross-post. Trying to fix the tags. :(

icons I wonder what splitting them out into a separate issue looks like? I can work on a "broken" admin header that is missing icons until we agree upon (or decide not to include) an icon set in core.

If you're going to 'eliminate subnav and implement set of "quick admin tools"' then why don't you use the same icons Mark has used in his designs for now? Working out what these need to be replaced with is an issue in its own right and there are a few potential approaches - we can update icons once this has been resolved.

It perplexes me that we have two groups moving forward on essentially the same task here though (Acquia are working on the header at the moment too, as I think was mentioned above), esp. given the scarcity of resources ... but I am far from understanding the mysterious ways of the Drupal community and appreciate the effort you're putting in yhahn :)

now I'm off to google smallcore (am a little afraid of what that might turn up!)

smallcore = the idea that drupal itself should be as small as possible (going as far as framework only) with all the bells and whistles being added from contributed modules.

(this is a "subscribe" post.)

A question in ref: to yhahns comment above - what's so bad about an admin theme?

Nothing wrong in particular, but it should be optional IMO. and if people like a theme so much, give them an option to use it for the full site.

And then there is a question of where it should show - backend is a vague term. Most people consider forums to be front end, yet node/add/% (for articles atleast) to be back end. where would node/add/forum or comments go? front end or backend? Having two themes can be jarring - especially if one is a bright theme, the other a dark one with things in a different location. All that change is confusing to users and I am a fan of consistency.

@ admin header - it may be an idea to have a "show all" option on menus where not all items are shown in the navigation. Maybe even some ui to promote/demote items?

And then there is a question of where it should show - backend is a vague term. Most people consider forums to be front end, yet node/add/% (for articles atleast) to be back end. where would node/add/forum or comments go? front end or backend? Having two themes can be jarring - especially if one is a bright theme, the other a dark one with things in a different location. All that change is confusing to users and I am a fan of consistency.

I know this is probably off topic but this is another issue that I am *very* interested in.

I think that 'where' (ie in what theme, assuming there are separate 'admin' and a 'site' themes which personally I am v much in favour of as a default for newcomers) should be defined by the content type.

I think this is a pretty conventional way to manage this issue. So, for adding comments and participating in discussion forums the default place for that would be from the 'site' theme (although presumably someone with access to the admin theme would probably have a backdoor/admin way to do this as well) and things like adding a news story would be an 'admin' task.

This is a big tangent from the issue under discussion, I know, but I can't help but take the opportunity to share my thoughts whenever I get half a chance :)

@yhahn, I think yoroy marked this issue postponed on the $page-top / $admin-tool / $closure + js/css issue because we need to resolve that before this can be committed, not because work can't continue here while that's being worked on. Since that issue is already at 50 followups, and this patch currently takes the same approach, for sanity's sake it's best to focus that discussion in one place.

David crossposted with me but fwiw I agree on the hook_menu() changes - we either need the entire admin tree available (like admin_menu - which already exists in D7 contrib and is likely to regardless of this patch, so why replicate it here?) or we need the 5-10 links in the d7ux prototype - which can just be a normal menu you can hide / move / delete / add links to same as any other. The user specific links / shortcuts from d7ux are a bit trickier, but those aren't covered by the hook_menu() changes here anyway (and can probably be done with a personalised interface and generic access callback - bit like per user block visibility although not that implementation with a bit of luck).

On icons - in this patch they're currently produced with a single CSS sprite, this means that contrib modules have absolutely no way to add their own menu items with icons to the header (except for adding their own css replacement probably which is going to be a mess), doing it that way is won't fix for me - hence wanting to move discussion to it's own issue, both to avoid this one being held up on it, and to avoid locking ourselves into something like that just to get the header in at all. This would either need magic naming and a sprite generator, or something like http://drupal.org/project/icons - both of those would be big complicated changes to core for something which needs more design discussion in the first place.

Subscribe.

I've been following this all day and just now made the connection to yhahn's admin module that was uploaded a few days ago. Usage stats already report almost 400 installations...

http://drupal.org/project/admin

Just FYI.

I am working on integrating this header code with the overlay code we implemented at Acquia. Since we base it on the popups module port we did to Drupal 7 (to fast track and leverage existing experience), we need the links to have the 'popups' class. So I've updated Young's latest patch to include the 'popups' class on links. These are the only two changes I made:

+function admin_menu_tree() {
+ // ..........
+ $user_links[] = array(
+ 'title' => t('Hello !username', array('!username' => $user->name)),
+ 'href' => 'user',
+ 'html' => TRUE,
+ 'attributes' => array('class' => 'popups'),
+ );
+ // ......
+function admin_menu_navigation_links($tree, $admin_only = FALSE) {
+ // ....
+ $l = $item['link']['localized_options'];
+ $l['href'] = $item['link']['href'];
+ $l['title'] = "". $item['link']['title'];
+ $l['attributes'] = array('id' => 'admin-link-'. $id, 'class' => 'popups');
+ $l['html'] = TRUE;

I am highlighting the changes, so they can be moved over to another patch, if Young is working on one in parallel. Also, I found that at least the default page.tpl.php was forgotten to include the $admin region output. That is probably a minor issue though compared to others raised.

FWIW, I am rewriting admin_menu to achieve the same in a design that has proven itself. So whatever you do here, please do it in a way that is optional. Thanks.

a popups/modal/inplace edit allover -soup.oi -a lesser known- soc. teen-20's oriented networking outfit
http://try.soup.io/

I guess that must be an example element of “Seductive Interactions" of Stephen P. Anderson, which Leisa referring to -if i did not get the concept wrong
http://www.disambiguity.com/random-inspiration/

it's just a good example -when its wisely used either on front-end or back-end

^{-my apologies for the pointer in a issue comment - I thought might be insightful while the work in progress}

Subscribe

Quick note: The latest patch lacks my changes adding class => 'popups'. I've added those to our repository at http://code.google.com/p/d7ux/

I apparently need to blog about this somewhere so I don't have to keep repeating myself but please, if you are going to use an external repository for collaboration on patches, you absolutely must make sure to cross-post patches to the issue queue whenever you hit a good "stopping" point, along with a description of the changes. Without that, the entire Drupal community (including both core maintainers) are essentially left entirely out of the loop until such time as the patch reaches a completely unreviewable size.

My sanity thanks you for your cooperation. ;)

@webchick: we are absolutely doing right that :)

Here is a screenshot of how it looks with Garland.

Going by the screenshot: Doesn't the submenu with Add, Edit, Dashboard, look weird under "Reports" being highlighted? That menu could have items under Reports menu itself.

@Gurpartap Singh: Both the top and lower menu items will be fixed. Think of the lower menu as a *toolbar* of some common used items. It is not context sensitive to the top menu.

Hm, I kind of agree with Gurpartap. I'd expect the lower menu to be an expansion of the upper menu (context-sensitive). If it's not context sensitive, I'd at the very least move the little triangle over to the left side, so that it's obvious at first site that the lower bar is a part of the triangle, not an expansion of the upper bar. One thing we've learned from our usability testing is that no one ever looks on the right side of the page. :)

For the patch itself - should the admin module be enabled by default in default.profile? (Or even in other core profiles too?) Also, comments should wrap at 80 characters.

Instead of using hook_init() to add your javascript/css, would it make sense to use #attached_js/#attached_css instead? That would seem to be a cleaner implementation.

Keep up the awesome work!

While I tend to agree that it is confusing for it NOT to be content sensitive, I think comments like that belong more at d7ux.org than here. This issue is about implementing what Mark and Leisa and the Drupal community come up with over there, not arguing with it and going our own way. See Leisa's comments at #27.

...I think comments like that belong more at d7ux.org than here...

I think that many people will take issue with such sentiment - the d7ux effort is good and all, but implementation will probably be carried out in the issue queue, so what is said here is important.

(Saying that, if there are people who provide a bridge between the two places, that probably won't hurt.)

Going on the admin heder issue of context sensitivity - the way it looks atm, I doubt anyone would expect it to not be context sensitive. If it is not meant to be context sensitive, then maybe it needs to be above the other row of menu items? Either that or it should only be shown when a button calling it has been clicked.

Note that earlier versions of this patch already had a context-sensitive lower menu, and there is discussion above regarding the scalability and usability of that approach.... see in particular comments #33, #35, #38, #46, etc.

(It is interesting, though, that at first glance, just about everyone does seem to expect this to be context-sensitive!)

Perhaps the real problem that we continue to bump up against is the "lipstick on a pig" issue brought up by Dries. The admin header was designed to be an optional feature (some above would say a contrib module) that builds on top of a more general reorganization of Drupal's menus. A reorganization of Drupal's menus (as described by Leisa in #38) to make them simpler would be a big usability win entirely in its own right, but it's a tricky problem for many reasons, and there do not seem to be a lot of development resources pouring in to solve it. Once/if that happened, though, some issues with the admin header would be easier - who knows, maybe it would even be simple enough that the scalability problems with a context-sensitive lower menu would go away?

I think what I'm trying to figure out is how, as a patch reviewer, I would proceed in trying to review this patch for D7? The code is starting to look really good (at least on a first glance), but the experience shown in #59 looks a bit like someone took the admin_menu module and broke it :) This is not surprising, since admin_menu was designed to work very well with the current Drupal menus and all their complexity, while the D7 admin header was designed to work with a different set of menus and menu landing pages that don't exist yet. Should we proceed on faith that the other parts will magically materialize and work correctly?

Not sure why many people feel that the lower menu should be context sensitive. I've been using applications on all desktop platforms which have a menu item and a toolbar. The toolbar icons are fixed shortcuts to certain parts of the menu, while menu items launch dialogs, submenus, etc. In this case, MBD suggests we should not have dropdown menus but instead have internal navigation on the dialogs/overlays showing up from the main menu items. Maybe that is why people think that somehow there should be "submenus" and if nowhere else, then that should be in what was set out to be a toolbar?

Office 2007/8 does have a context sensitive toolbar... and I *hate* it ;)

For the admin header to be useful for me, I can only imagine the toolbar to be context sensitive to the menu item selected in upper menu. Isn't an admin tools supposed to speed up tasks? If I have to reload a page for each top menu item click, then, in my opinion, we are missing a point.

If we really need a sticky toolbar for specific items, we can just have a default top level menu which contains those items. the behavior will be same as current, plus the admin.module like behavior.

Please don't take this as any offense. I really appreciate the effort, and I spoke what I realized.

> Office 2007/8 does have a context sensitive toolbar... and I *hate* it ;)

Office is a desktop application, I don't care for it's application of the toolbar. But in case of websites, a utility like admin_menu really comes useful, and it being rendered as the proposed admin header in core itself will also look awesome and provide a good experience (though it trims some level of menus than admin_menu).

@Gurpartap Singh: take this admin header in context with the overlay. There would be no submenu items below the main menu items, at least they would not show up in traditional menus, but would be accessible via navigation on the overlays. See mockups on http://d7ux.org/content/ for example. (All this would degrade to a similar looking but non-JS experience for people without JS).

Seems to me there are ways to kill two birds with one stone. Keep the shortcuts, but also make submenu's dependant. I'm not suggesting icons for every sub-element - only those which deserve an icon. For example:

All of this is overlooking that modules can add further top-level items. Likewise, if those toolbar links will be dependent on the page context, this entire admin header will take up half of the window height. Additionally, the mockup in #70 clearly shows that the icon shortcuts don't make any sense on administration pages (whether being in context or not).

Anyway, that's my last comment here. I've recently learned that my core contributions are unwelcome.

@sun: Not sure where you've learned that your core contributions are unwelcome. I think we are debating what to show at all still. What Noys suggests seem to imply that contrib modules would not provide icons, not that they would not provide menu items at all. Not providing icons might make them categorized as "less important items" (not deserving an icon), which I'd contest, since for some people Panels or Views could be their most important structural administration item for example.

@ sun: Your core contributions, as well as the countless (and I mean that—countless!) contributions elsewhere, are most definitely acknowledged and appreciated. I think your voice in this thread has been helpful as well, and I would be sad to see one voice be drowned out.

Compromise is part of doing open source projects, as I'm sure you well know, and meeting in the middle on some of the issues surrounding the admin header would be much better than diverging. I personally use admin_menu on pretty much every Drupal site I run, but it does have some flaws; in my ideal world, I'd have an admin header that you set whether you want it to be the admin-menu style, or the 'prettified' admin menu discussed here. That way John Doe the content manager can have his gooey GUI, and I can have my workhorse admin shortcut menu.

I already reviewed the contextual links in #33 but apparently some people missed it. If you don't have access to every page in those links (which you don't, look at the hook_menu changes) then it's more clicks and more page reloads to get where you want to go. Horizontal contextual links are never, ever going to scale with admin/settings - and part of process of streamlining user/content/site building etc. is stuffing more and more lesser-visited pages into a big settings dump somewhere - that section is already bigger in D7 than D6 . So we're then left with the top-level + shortcut links proposed by d7ux or simply putting admin_menu into core . The half and half which was in the original patch would be the worst of both worlds IMO.

I agree with David that adding the header without any other of the proposed changes makes no sense at all - and I'm disappointed that so much effort is being put into the pretty without the context it requires to have a chance of working. The only patch I'm aware of in the queue which would help with some of the bigger IA changes is #492834: Hover operations for flooded state screens if one of the top level links goes to something like that page, then suddenly it all becomes a bit more scalable.

I'm going to ignore the apparently controversial user interface in this review for a strictly code-only review:

modules/admin/admin-toolbar.tpl.php:
- Drupal typically uses double quotes for enclosing attributes (e.g. class="toggle... rather than class='toggle....

modules/admin/admin.info:
- Most core modules don't surround the name/files/core version with quotes as it's not necessary. SimpleTest is an exception, and I'm starting to think that that's a core bug.
- Shouldn't specify the version explicitly in the .info file for core - it should just be version = VERSION to take on the version of Drupal core as a whole.

modules/admin/admin.module:
- You call admin_is_enabled('admin inline'), but that is unlikely to ever return TRUE because the permission "admin inline" is never defined in hook_perm()?
- Perhaps, to be consistent, rather than checking the path for 'admin', the code should copy the conditional in system_init()? (if (arg(0) == 'admin' || (variable_get('node_admin_theme', '0') && arg(0) == 'node' && (arg(1) == 'add' || arg(2) == 'edit')))).
- I mentioned this before, would it possible to bypass the hook_init() implementation entirely by using #attached_css/#attached_js? Drupal_render() should now check for those and attach them if they're present.
- Drupal now always has a space between concatenation operators (.) and strings.
- Comments should wrap at 80 characters.
- I question whether the function admin_in_active_trail() should belong to the admin module - it would seem this would belong more in menu.inc?

modules/admin/admin_toolbar.css:
- Generally needs coding style work. In particular: } doesn't need to be indented, CSS definitions should not be placed in a single line (for example: div#admin-toolbar .collapsed { display:none; }, there should be a space between property and value (color: #ccc; rather than color:#ccc;), comments should start with a capital letter and end with a period.

I'm not sure what the policy is on this, but I'm not sure if IE6 fixes should be put in a separate file(?)

That's about it! The overall code structure and the javascript looks very good; there are just a few coding style problems to work out and then this will be ready - at least in terms of code.

Many of the initial comments above relate to the original implementation of the header, which was using the second level as a context sensitive list of items. This is not how it is implemented anymore, so those concerns do not apply.

There are new concerns regarding the looks maybe suggesting it is context sensitive. :) I am sure Mark and Leisa are aware of this concern, but I am not sure whether it is rooted in our pre-existing experience with the Drupal admin menu, and not expecting a set of toolbar items at all in that place.

There were also concerns regarding the required nature of this. Well, as it is right now, it resides in its own module, so it can be disabled altogether. Other modules such as admin_menu can provide power-used replacements.

When enabled, the top level items show the top level of the main administration menu tree. If you customize that menu, you customize this menu too. The whole administration menu is planned to be reorganized (AKA new IA for the admin area), which would hopefully result in a more flattened admin structure. That, as many people noted would not be addressed in this patch at all, and we either have faith in that it is going to happen or postpone this issue on that. I don't think it is worth it to postpone this one on that, since having the header in core would help us reorganize the admin tree and see how it changes are speed of using the admin screens.

Also, when enabled, the lower items would use a custom menu from the menu admin area. There will be different sets of custom menus for different use cases, and whoever can customize menus will be able to customize this menu too. How randomly added items get icons is a good question, but we are not addressing icons here.

The admin header in its current state can be considered as shortcuts in two levels. The top level is shortcuts to the main admin menu structures, while the lower level is custom shortcuts you can customize for your needs. The upper menu sort-of guarantees that you cannot "edit out" certain parts of the admin menu, which you then cannot access anymore (although you can still edit that menu via reorganizing your admin area). The lower part gives you quick jumping points to commonly used items.

I think this concept is useful as it is without the IA improvements, but its true usefullness certainly only comes with the IA improvement in place.

I'm also inclined to get this committed. We need to make a start somewhere, and build/iterate on that. Like always, it is about incremental improvements and breaking a large problem up into smaller pieces.

Could someone re-roll a fresh patch from whatever's in the Git repository (along with instructions for testing) so we can take a look at where things stand?

@webchick: the google repo we are using is Subversion based (Google only supports Mercurial and Subversion and we had a strong feeling that using Subversion would be easiest for collaboration in terms of not getting to know a new tool for many people).

Here is a quick reroll of the header patch. It has a small list of changes since the patch in #55 which I also attach. Those are all the changes in our repo since #55 was rolled (but all of them are rolled into this attachment). Images should be the same as in #55.

How to apply this patch:

- Get images from #55.
- Apply http://drupal.org/node/468534#comment-1687578
- Apply this attached patch (the bigger one).
- Enable admin module on your Drupal 7 site.
- Try the menu.

I haven't reviewed the patch yet, only the discussion in the issue queue. (I've also re-written this comment a few times.)

If I understand this module correctly we are effectively introducing a desktop application kind of view to the admin section. We are adding a 'menu bar' which will read the structure of the admin menu and ddisplay it in dropdown menu fashion, analogous to the file, edit, help menus in a desktop app. We are also adding a toolbar with links to the most commonly used functions, analogous to the opens, save, print buttons in a desktop app. If I've missed the boat on this analysis then the rest of the comment can probably be disregarded.

I think the concept has lots of merit, especially for making drupal more appealing to less technical users. Giving them a paradigm that they are familiar with is a good thing. My only concern is that we all expected the two menus to be linked as that is the typical web behaviour when it comes to navigation. The question we need to ask is whether non technical users will expect the menus to be linked, or if they can understand the desktop metaphor we are proposing.

Why are we duplicating the code of admin_menu? I know everyone likes to write their own version of a function as they think they can do it better (please do not look at this as an attack on anyone specifically, but as a general view on software development in general, and OSS specifically.) Admin_menu has been around for years, has an active maintainer and is known by many people. I can understand sun's frustration at a (probably unintentional) attempt to replace the code he has worked on with something newer that doesn't have the testing. I think we need to get out of the habit of reinventing the wheel each time we add functionality.

I wonder also if the two menus, being independant, should be able to be enabled or disabled individually, either as blocks or via a setting.

I hope this make sence, being 4am on the train to work.

Soli Deo Gloria

@wretched sinner: we are not introducing any drop-down menus of any kind. The top level menu items will not drop down to more menu items, they will lead to actual actionable admin pages. The d7ux.org site has some of those actionable screens detailed. The goal is to get people to the action fast without rolling dozens of menu items by their face. Since improving all of the admin landing pages is not possible in one patch, we needed to start somewhere and this was one of the logical points to start at. This header is one part of a set of changes we are working on for Drupal 7.

Given that we are neither dropping down menus, nor making customizations to the administration menu tree like admin_menu module does, but we are presenting one top level of the management menu and one set of custom items, the later which will be pulled from one of many custom menus, we have little overlap with what admin_menu does. Depending on how the administration menu flattens in Drupal 7, maybe admin_menu module will look more similar to what we have here, but it will certainly keep many of its power-user improvements. At this point, they look pretty different in their goal to me. Admin_menu caters to the power users, while the admin header here caters for those who are right into a new Drupal installation and need to find their way easier. To cater for many more audiences, as said above, you'll be able to disable the admin header and enable any replacement, such as admin_menu module.

Thanks Gabor. I haven't been following the d7ux effort as clusely as I would like. I was a little confused from reading through the issue queue and your explanation makes sense.

Soli Deo Gloria

(Note: I tried really hard not to read too many of the comments here so that my mind could be "fresh" going into this. I have a feeling this is regurgitating a lot of what's already been said, and I apologize in advance.)

Here is my review based on the current work, including the modal dialogs. I've encoded the review into screenshot form as well so that those who can't get the patch working can see what it does. Click for full-sized versions.

My biggest problem is that the visual proximity between the top-level bar and the "shortcut" bar below guides my brain to make the connection that they must be related to one another. From the sound of it, others in the issue are also making the same connection. When I contrast what's in this patch to what's on my Desktop, however, you get something that looks like this:

So I wonder if re-locating the shortcuts bar somewhere else (along the side, at the bottom, etc.) might help with this dissonance.

The second problem I have is with the "Edit" link in the shortcut bar. Edit *what*, exactly? Again, that darn contextual-seeking part of my brain expects this to be editing whatever I'm looking at on the page right now. So node/%/edit for a node page, and admin/build/views/%/edit for a view page, and hidden when there's nothing that can be edited (such as an admin page). Alternately, my second guess would be that this would kick me into "Edit mode" where I could start doing things like dragging/dropping blocks onto the page and doing inline editing of my content. When it instead sends me to admin/content/node, I get very disappointed and sad.

I *am* happy to see the icons gone, as well as the admin theme, since those are both something we'll definitely want to deal with separately. And I do agree that if we try and handle the various menu-level structural changes in this patch along with the visual styling, we're going to be here for quite a very long time. Both of those make sense as separate issues. I tried to keep my review to the behaviour of the bar only.

The big question we have before us is whether it's worth committing this patch (once it's RTBC from a technical perspective) with some of these concerns so we can further build on it, or whether it's worth spending some more time on this, esp. regarding the concerns that we're currently slapping "lipstick on a pig" since we are not at the same time tackling the underlying IA issues that Mark and Leisa's work has helped address. I'm a little torn on this point, myself. I need to go back and review the comments in this issue now that I've tested the patch, and will post a follow-up tonight.

@webchick: for comparision with your Mac screenshots, here are screenshots from two other major OS platforms showing menus and toolbars/shortcuts. They are in close proximity and they have nothing to do with each other (no context sensitivity).

That all subitems of an area would be accessible via tabs on the overlay might help people grasp that the lower part of the menu is not for subnavigation. Have you tried the "Menus" item under site building for example? All the subnav shows up there on the overlay. (Warning: the click path to edit menus will probably not be like it is at the moment).

"Hello" overlapping "Reports" would not happen once/if an improved admin IA is in place, since there will be much shorter and fewer top level admin items. See almost any d7ux.org mockup.

Top level items looking plain and ugly is also planned to be solved via actionable forms showing up for the main items. "Add" and "Find content" is covered here: http://www.d7ux.org/content/ These are not among the top-top level items, and I was intending to lead you into the People mockups, but they are not appearing currently. Anyway, hopefully the plain old ugly admin subsections problem is temporary.

On the "Edit" item, the mockups/plans actually get it to let you do edit-in-place, or if the page is too complex, open editing for you in an overlay. What's implemented in our repository at the moment is that if you are on an actual node page, the Edit button opens up the node edit form in an overlay for that node. This needs some funky server/client side scripting to alter the item properly given some context, since the different items will not be possible to define in the menu admin UI, given how subtle that should be. We might need to wire this item to a special path, so we can clearly work with and alter that. It is currently a subpath of the main Content overview page, so it falls back on that if it has nothing else to do at the moment. I totally agree it is not ideal.

BTW for anyone wondering the source of these screenshots showing how this header looks in context with the overlay and some node form changes: http://code.google.com/p/d7ux/source/checkout then install with the d7ux profile, and click around. *Not* that this means the header should not make sense without the overlay. The intention is to couple the header with an admin theme (not in this patch, but in a separate one), which would make your admin pages look nice and simplified like in the overlay even if you are without JS. You'd experience the same reinvented IA and admin screens. (The d7ux repo linked here actually uses a different theme for the overlay, which was based on the d7ux.org mockups).

The difference between the screenshots in #86 is that with the unrelated menus, nothing appears "selected" in the upper menu. The fact that "Site building" appears "selected" in the current admin header, makes me think of it as a tab, where the secondary tabs change depending on the selected primary tab. It also doesn't really help that the lower header is connected to the upper header on the right. I'm all for this patch, but I think it needs a few minor design/theming changes before it's easy to understand at first sight.

Instead of hard-coding a custom variant into admin.js, we want to ship with and use one or the other of the available jQuery plugins for managing cookies.

The other real difference is that menu items such as FILE, EDIT (...alll that follow), are so common that one expects a drop-down of choices to follow - thereby allowing a shortcut bar to seamlessly fit in. I'm not sure this is a fair comparison.

While I cannot say that I'm a fan of the organization of the Admin Menu, I do appreciate how fast it allows me to work. Tasks that I can do in one click, will take 2/3/4 delayed clicks without a submenu. For example: with the admin menu installed, after clicking content, I'm immediately presented with sub-choices to view, add or create content types. With the d7ux header, after clicking content, I have to wait for the find content screen to show itself, then I have to click add content (2 delayed clicks vs 1). If I'm to add content types, that's even more clicks. I don't believe this is privileging the content creator.

I love the idea of shortcuts. But I also think it's necessary for such a feature rich site to have sub-menu's. IMHO, if config settings weren't peppered throughout the admin menu, and if the links were bigger, it'd be a pretty usable tool. Or, even better, if the d7ux header would allow for sub menu's and shortcuts (see #70), it'd be even more usable.

Is top level menu sensitive icon toolbar out of discussion now? *Really*? What a fail for me :(

Ok. Just got done with an hour+ long discussion in #drupal-usability, and reading all 90+ replies. I'll try and summarize, but this will probably be a bit long and rambly.

Basically, we find ourselves at this precipice where what we decide to do in this issue ultimately decides, to some extent, the fate of the D7UX work done to date, and the tone for how these issues will be dealt with from now until code freeze. So far, exactly ZERO of the direct, in your face, content-creator-empowering work that Mark and Leisa have slaved so hard on over the past few months in Drupal 7. There's been a lot of important "plumbing" work, such as moving content and help to blocks, but this is really the first issue we have something that directly impacts each and every Drupal user. This is, imo, the big reason why you find this issue at 90+ replies. A patch in the Drupal core issue queue has a way of making things a lot more "real" for the Drupal community than Photoshop mockups.

There are a couple of competing forces at play here:

#1: HOLY (*@&ING (*&#$ CODE FREEZE IS IN TEN WEEKS!!!
#2: Design without usability is just "lipstick on a pig." The patch puts a nice drop-shadowy face on our existing admin structure, without doing anything about the underlying horrifying rats' nest of links that lie just beneath its surface.
#3: Won't someone please think of the kittens?

I'm sensitive to the comments Leisa's made in this issue. It's really tough seeing your work dragged out in front of the Drupal core's infamous "Flaming Hall of Judgment," particularly when you feel you've already done that song and dance once on d7ux.org. It has to be further frustrating to see people with no UX background questioning things that you've done, and seemingly dismissing the substantial amount of research you've put in. (I should point out that we're really not trying to do this, and are never intending to be disrespectful, however. We're merely trying to understand. This is brand new territory for us, too. :))

Despite that, I do find myself siding with Dries and Gábor -- doing the "code freeze freakout." We need as many people to test this as possible, as fast as possible, because pretty soon Drupal's default behaviour is going to be locked down for 3+ years. Holding this patch until all the various dependencies -- an icon strategy, a method for attaching icons to menu items, a re-worked administrative IA, new and fancy landing pages for each top-level section, etc. -- are in place sounds like a recipe for disaster. I forsee it requiring rush-committing basically everything at once in mid-August and praying that we can fix the fall-out issues in two very rushed weeks leading up to Drupalcon. Not my idea of a fun time, and certainly not the legacy I want to leave behind for the D7UX project.

Leisa has said:

"If we say that the header Information Architecture maps exactly to current core Information Architecture AND the icons are removed from the second level of navigation then I can see this as a repositioning and 're-colouring' of the admin navigation and I can live with that as a first step to the d7ux header."

If she can live with that, I think that's what we ought to do, and for the most part seems like what this patch is doing.

So. Let's talk about what needs to happen for this patch to become "RTBC." I haven't had a chance to do a code review yet, so this is more at a conceptual/design review stage. I'll try and do a code review this weekend during the UX sprint (though in general I will say that I'm concerned with the issues sun is raising, since he has the most experience of all of us at implementing sticky admin headers).

So really, these are mostly questions for Mark and Leisa:

a) Right now, all of the top-level menus go to a screen with a list of links and link descriptions, like this:

Is that what is ultimately intended? I can't really find any mocks for these at d7ux.org, apart from some very specific ones such as the modules page.

b) Gábor said:

"Hello" overlapping "Reports" would not happen once/if an improved admin IA is in place, since there will be much shorter and fewer top level admin items. See almost any d7ux.org mockup.

I find this a bit naïve. If we implement the underlying admin IA according to the mocks, we end up with 5 top-level links rather than 7. So yes, that'll help a little. But unless we're planning on removing the flexibility for contributed modules to add additional categories here (such as "Development" for modules like SimpleTest and Coder), this is definitely going to come up "in the field." So what's our plan for dealing with this?

c) It does concern me that basically every single person in this issue since the change from admin module's "figure out an icon for every sub-task" approach to the new D7UX "secondary nav = a static shortcut bar for commonly-used tasks" approach has assumed that the shortcut bar was contextual. One could argue that this is because the icons are missing, which is why the desktop app mapping didn't immediately happen for us. But I remain unsure. Is there user research that says conclusively that "real" users do not have this problem? If so, why are we having such a hard time with it? Is there something else that can be done visually to help further enforce the notion that those two menus have absolutely nothing to do with one another?

I will try to rephrase a bit, adding related questions to a) and c) .

a) Where are the 4/5 large-scale screenshot for those pages in the navigation. Since - how can we link to something we have no idea about?

c) Can you provide user data (or something), that people understand the second bar is not contextual? (We don't understand how being contextual helps, unless a) provides answers)

Although Leisa set out to say, that this is fine - we need to be very aware of the fact that unless IA changes and Icons go in - this is not core worthy.

Two simple comments after reading through the past fifteen replies (I read through the rest earlier):

1. The second level feels like it should be context-sensitive to me, because it matches the style/feel of the top level so much. That's just my opinion. Either way can be learned, but intuitively, it feels weird the way it is.

2. If there are more top-level items then there is space (i.e. the "Hello" overlap), they should break to a 2nd line, imo, or have a double arrow thing you can click on to view the rest (like in Safari 4, when you have more tabs open than your browser tab bar allows for).

@webchick/Bojhan: On (a), the top level items from the IA are "Content" "Structure", "People", "Appearance" and "Modules&Config". Corresponding mockups are at http://www.d7ux.org/project-framework/ for some of the items. The People mockups used to be available but looks like it was removed for some reason. Content and Modules and Config are there (although for Content we might not have a starter screen for the top level item, but only for Find content). Appearance and Strucuture is being done know, since the grand plans for a totally new structuring tool where postponed until a later Drupal version on the structure summit. I agree that not all areas are clear, and asked Leisa to provide a detailed mapping of the old IA to the new IA plan.

@sun: agreed with you on the cookie plugin.

Citing cwgordon7 from #88 concerning c) from webchick's #92:

The difference between the screenshots in #86 is that with the unrelated menus, nothing appears "selected" in the upper menu. The fact that "Site building" appears "selected" in the current admin header, makes me think of it as a tab, where the secondary tabs change depending on the selected primary tab. It also doesn't really help that the lower header is connected to the upper header on the right.

- It looks like we have some engineering work to do -- @yhahn is going to take another crack at it over the weekend.

- It looks like Mark and Leisa need to take the feedback in the comments above as input and decide whether they want to do something with it or not. That decision should be shared, along with some rationale.

The second-level items/links cannot be context-sensitive, because that would mean that we force and limit Drupal's menu system below admin/* to a max-depth of 3 levels (including tabs/local tasks).

@sun: well, following the same logic, if there are no dropdowns and the second level is not context sensitive, then we limit the levels to two, no? Looks like the d7ux.org mockups show levels on the top level and on second level as tabs on the overlay with no third level indicated. Because that sounds quite a bit of a stretch with a typical Drupal installs with new subsystems like a store or groups or multilanguage features, it would indeed be great to see the IA mapping of the existing core IA to the new one as mentioned a few times above, so we can see how core would be suggested to reorganize and how contrib would fit in.

Broken bot.

persistent bad bot.

Actually, in this case the bot is right, since that second patch will not apply.

Here's a re-upload of the "real" patch without the "diff" patch (in the future, maybe name that with a .txt extension). That should stop testing bot from freaking out.

I have reinstalled http://admin.drupal4hu.com/ see http://drupal.org/node/484820#comment-1677144 for instructions.

I do not see anythingicons in the header region, though. I have untarred the sprite.png from June 14 in modules/admin. Firebug does not show me any 404. Help?

Edit: i see a few links pointing to a hodgepodge of targets after enabling the admin block in the header region.

I think the icons were removed.

@webchick, Bojhan - So after a weekend with Mark, Dries, Yoroy and Leisa I think I have a good grasp what it is now about so let me immediately share that knowledge - so here goes my "as far as I understand".

a) The hubs Structure and Configuration & Modules will look like now - an index of links. Probably visually a bit different, but content wise it comes down to that. The People one is already on D7ux.org, and also the Content.

b) The intention is that top level categories made by modules are not allowed to add "Development" in the top menu. Instead they live on Configuration & Modules or on Structure. For larger applications such as Ubercart/Project module, they will be linked in a box on Dashboard and Configuration & Modules.

c) There has been some research, but by far not indepth enough to make any conclusion on whether users are (not) confused by the second level not being contextual.

So why?

Leisa should be posting a blog post soon, but it's mainly about differentiating settings from actual site building. We will need modules to see how far this concept scales - but it is fairly fixed. And tuns to Structure and Module & Configuration to actually map the content.

The icons were removed, and left for a follow-up issue. This patch focuses on the header, and follow-up patches will focus on icons, IA, etc. Step by step! ;-)

b) The intention is that top level categories made by modules are not allowed to add "Development" in the top menu. Instead they live on Configuration & Modules or on Structure. For larger applications such as Ubercart/Project module, they will be linked in a box on Dashboard and Configuration & Modules.

That makes this admin header only suitable for the "default" use-case, whichever that is, which seems to be presumed here. Ubercart is a nice example, because its top-level category/item is the central working space for site owners/admins/editors of a e-commerce site using Ubercart allowing to perform common, day-to-day tasks. And that's just one example of unlimited use-cases for Drupal. This sounds like a use-case-specific implementation, and such limitations usually live in contrib.

@sun Yes. But either way, the menu is not context sensitive so even if you have 10 top-level items up in that menu it would still take you to the section you need to be in. Right now the way around this is, to either click this top level on the dashboard or on Configuraiton & Module.

The section where you need to be in, still exists in D7UX concept and is the same as Drupal now. I also think that the second-level nav can link to Ubercart, or Development since its role/user based. So day-to-day tasks have to be exposed there, for those high level modules.

Remember the principle of 80%, we can't battle that principle by saying 80% should live in contrib. There are indeed with large applications many considerations to be made. But the find ability of site building versus configuration is by far improved with this concept, so saying there is absolutely no trade off in terms of efficiency is bull - but it should be better for applications.

Nobody likes anything, that is truly fixed - so maybe we should open the possibility. But I would rather not have 5 more top-levels up there.

Not to make this issue longer, but a problem I see is that the edit button is contextual in the shortcut bar, but the other buttons aren't.

We should probally split the actions bar into a new issue.

TBH I'd be pro splitting the action bar until another issue. Then this patch can probably go in very quickly and we can get some eyeballs on it.

Poor yhahn/Mark/Leisa. :( Your work is being systematically dissected. But trust me, there is a method to the madness....

I spoke to Leisa very briefly during the UX sprint in #drupal-usability and she mentioned adding a 'customize' link to the action bar (I assume action bar == shortcuts) - that's not in this patch and seems like a good reason to split, I also think that link explains why they aren't contextual.

I'm OK with splitting the toolbar to another patch. I had suggested that before.

@sun/Bojhan: the d7ux guidelines Bojhan explains are the guidelines. It does not mean there will be no technical way to add menu items in the top level menu, so those, who want their site pimped up with more top level items can go in and modify even with a click and submit interface (menu editing) or via code (add top level items under /admin). So *you* will be able to customize it for your liking. The main question is whether it is good as a general guideline. How many of such big subsystems would one install? What do you think? A store and a CRM system, and ...?

Young does not have the sufficient time before the weekend, so I am taking on resolving the feedback. Will post a patch soon.

Just so we don't get all confused, can we get our terminology straight please; and avoid vague terms like 'action bar'? May I suggest 'menu bar' for the one at the top. And 'shortcut bar' for the bigger one at the bottom ('toolbar' is a misnomer). (Some people are using these terms already.)

It would also be useful to keep this thread as a general admin header discussion, and then create sub-issues off of this one. That will give us a central place to track the issue and discuss overall concerns. Then the sub-issues can be where we concentrate on code reviews; which are currently being drowned out.

So create a new issue for the 'menu bar' and then put a reviewable patch in there.

Is that a good plan?

@ dmitrig01 #114: The edit button is not contextual; it just looks like that. Which may well be a minor issue.

All: http://www.yoroy.com/2009/reorganize-drupal-admin-items-within-d7ux-fram... provides a nice video overview of the new proposed IA and how it maps to the admin interface interaction. Make sure to review that. For those lazy to click through:

@alpritt: Most of Young's code use the "toolbar" term for all of this admin header. I decided to keep it while reworking it based on feedback above, so we don't get too even more changes. There will still be many things including naming to obsess about, so even if I remove half of the patch which handles the "shortcut bar", we still have plenty of stuff to obsess with unfortunately.

@alpritt: And yes, the edit button would be context sensitive, it would for example let you edit a node in place and/or in an overlay if you are on a node page. That is however not to be solved in this issue, especially since we remove the shortcut bar for hopefully quicker acceptance.

For @sun's suggestion in #89, I've added this new issue: #507072: Add jquery cookie library.

New patch still with both bars. (The reason I did it this way is that it makes it easier to sync the changes and we can separate them with less pain hopefully). Also, a complete version of the header is documented this way without me throwing off half of the module first just to get something committed finally. So here are the changes:

- $admin_links renamed to $admin_shortcuts everywhere, see alpritt's notes above
- Module and the whole admin bar now consistently called the "admin toolbar", the upper part is the "admin menu" while the lower bar is the "admin shortcuts"; the module used to call the bars the admin toolbar so that was simply kept and made consistent
- HTML quote errors fixed as noted by cwgordon7
- .info fixed per cwgordon7; also, package set to Core
- added many @file comments
- removed admin_init() in favor of #attached_js and #attached_css, per cwgordon7
- moved theme preprocessor for admin toolbar into main module; it was just a few lines of PHP code
- removed admin_is_enabled() in favor of user_access(); the previous function had a complex and custom way for themes to say they do not want the admin header; themes can still preprocess it out or override the theme, so they have all the power to disable it; I believe this function was mainly for the other, now removed features of the module (in-place edit, etc)
- added "to-overlay" class to items, marking them to be opened in the overlay; this will only make sense once the overlay is in, but since we are progressing there it was logical to include it here
- menu tree handling changed per Dries, so only the items right below "Administration" are included, the Add new content item is not there
- added lots more comments to the menu handling code and referenced the base implementation in core, based on discussions with pwolanin, so we clearly see what is done why
- fixed CSS coding style issues mentioned earlier
- per sun's suggestion, the code is now using jquery.cookies.js; see #507072: Add jquery cookie library

Some concerns which were not solved and might not be solved either:

- patch still depends on a region to put the header in (see below), there was heated debate on adding regions or not
- IE6 fixes are still in the base CSS
- admin_in_active_trail() is still right in this module, not in a system file; this can IMHO be refactored later
- finally, the shortcuts bar is still here with all its controversy; frankly I think we give ourselves lots of extra work by splitting it off, since many of the infrastructure built out here is not needed if we only need one level of items, and some abstractions will make that much less sense; anyway, looking into submitting a limited patch pretty soon (in the coming hour or so).

To test the patch:

- apply #507072: Add jquery cookie library and http://drupal.org/node/468534#comment-1687578
- apply the attached patch
- get the image from #55
- enable the admin module
- enjoy

subscribing

I got confused by the context sensitive / not context sensitive behaviour of the edit button but yoroy and Gabor clarified in #drupal-usability.

What it's not: a replacement for node/%/edit when you're on a node page

What it is: an action button which enables an 'edit' mode and/or an overlay for editing elements of the current page. So whatever has either overlay / inline / or views-style clicky linky editing you'd be switching that on or off with the button. So this needs to be considered in terms of #473268: D7UX: Put edit links on everything, not in terms of edit tabs.

I committed #507072: Add jquery cookie library which affects the instructions in #124.

Ok, you've asked for it, so here is the "minified" version of the admin toolbar.

1. Lacks a shortcut bar, so consequently, lacks an install file, because it works off the admin menu only; lacks a JS file because it lacks a toggle; CSS file, theme file and PHP files are smaller.

2. Less dependencies: now it only requires the page top region from http://drupal.org/node/468534#comment-1687578

3. Still uses the sprite image from #55, which *does* include imagery for the shortcut bar and the toggle. I sincerely hope that the shortcut bar will get in, so I think it is pointless to take time with redoing the sprint and recounting the pixels for the CSS. Not something I can do quick (unlike dropping files or big chunks of code). If we need to do that, I can take time on that too.

To test:

- Apply http://drupal.org/node/468534#comment-1687578
- Get sprite image from #55
- Apply the attached patch
- Enable admin module
- Be happy!

You should see something like this (no second level bar, no icons, no Add new content link in header):

First code review:

1. This patch is actually pretty simple!
2. I'd add a TODO that mentions that admin_enable() is a temporary function until we have made it configurable. In general, make sure to add TODOs at the appropriate places. We had some TODOs in Fields API too -- as long we don't exaggerate with TODOs I'm happy -- it allows us to make progress.
3. For the time being, let's kill the 'Edit' shortcut from the shortcut bar. We can add that with the 'Edit in place' patch.
4. I'd prefer to call this module toolbar.module but that is minor.
5. admin_toolbar.js/css should be admin.js/css or toolbar.js/css
6. Let's simply use 'overlay' instead of 'to-overlay'
7. It is not clear what this is for: + $vars['classes_array'][] = 'admin-toolbar-links';
8. + * Based on menu_navigation_links() with some admin toolbar specific changes. I'd be a bit more specific and mention that we're adding icons and overlay links.
9. admin_in_active_trail() should probably have a TODO and a follow-up issue

Great job. I think we're close, and it is sufficiently small and straight-forward that we should be able to flush out the remaining issues. It is not necessary to split out the shortcut bar -- it is more trouble, and we can always decide to remove it later, if it doesn't test well. Let's focus on making some progress and enabling more people to help and test.

Hopefully I'll be able to commit it this week so we can start making progress on implementing the IA. It certainly looks like that is in the cards.

Amendment to Dries' feedback: Proper syntax for todos:

  // @todo In general, make sure to add TODOs at the appropriate places. We had
  //   some TODOs in Fields API too -- as long we don't exaggerate with TODOs
  //   I'm happy -- it allows us to make progress.

I also agree that "Toolbar", resp. toolbar.module, would be a more appropriate name for this thing.

Furthermore:

+name = "Admininstration toolbar"
+description = "Toolbar exposing the top level administration menu items"
...
+files[] = "admin.module"

Don't use quotes for .info file values unless technically required.

+    'admin toolbar' => array(
+      'title' => t('Use administration toolbar'),
+      'description' => t('Access the persistent administration toolbar on each page.'),

Internal name should be "access xyz". Please squeeze a "displayed" into the description after "toolbar". And s/each page/all pages/.

+  $items = array();

Technically not required.

+ * Add admin toolbar to the page_top region automaticaly.

Typo in "automaticaly".

+    $page['page_top']['admin_toolbar'] = admin_toolbar();

That function doesn't sound like a function that builds/renders/outputs something.

+  if (user_access('admin toolbar')) {
+    $vars['classes_array'][] = 'admin-toolbar';
+  }

This does not ensure that the class is indeed added to the BODY tag.

+body.admin-toolbar {
+  padding-top: 30px;
+}

That will break any absolute or fixed positioned elements in a theme.

+  $module_path = drupal_get_path('module', 'admin');

$module_path is only used once.

+  $tree = admin_get_menu_tree();
+  $links = admin_menu_navigation_links($tree);

$tree is not used elsewhere.

+  global $user;

Should be moved to the top of admin_toolbar().

+      'title' => t('Hello <strong>!username</strong>', array('!username' => check_plain($user->name))),

Should use the appropriate placeholder instead of a manual check_plain().

+      'href' => 'user',
+      'html' => TRUE,
+      'attributes' => array('class' => 'to-overlay'),

The regular user account page should not be displayed in an overlay.

+  $user_menu = array(
+    'account' => array(
...
+    ),
+    'logout' => array(
...
+    ),
+  );
+  $output['user_menu'] = $user_menu;

$user_menu variable is superfluous.

+  foreach ($tree as $k => $item) {
+    if ($item['link']['link_path'] == 'admin' && !empty($item['below'])) {
+      // Only take items right below the 'admin' path. All other
+      // management items are discarded.
+      $tree = $item['below'];
+      break;
+    }
+  }

$k is not used in this foreach.

+      // Only take items right below the 'admin' path. All other
+      // management items are discarded.
...
...
+      // Make sure we have a path specific ID in place, so
+      // we can attach icons and behaviors to the items.

Does not wrap at 80 chars.

+      $links['menu-'. $item['link']['mlid'] . $class] = $l;

The generated classes from the $links array won't be very useful. Since you process each item anyway, I'd suggest to use a part of the link path instead. However, also keep in mind that modern browsers can use modern CSS to apply styles to [href=xyz].

+div#admin-toolbar,
+div#admin-toolbar * {
+  margin: 0px;
+  padding: 0px;
+  border: 0px;
+  outline: 0px;

"px" is superfluous. (and elsewhere)

+/**
+ * Base styles
+ */

Missing period. (and elsewhere)

+  color: #ccc;
+
+  position: fixed;
+  left: 0px;
+  right: 0px;
+  top: 0px;
+
+  z-index: 100;

Pointless blank lines. (and elsewhere)

+div#admin-toolbar div.admin-menu #admin-toolbar-user {
+  position: absolute;
+  right: 10px;
+}
+
+div#admin-toolbar div.admin-menu #admin-toolbar-menu {
+  position: absolute;
+  left: 10px;
+}

Those menus will most probably overlap/clash when not browsing with a maximized window.

In general, all CSS should follow our CSS coding-standards; specifically: styles/rules should be ordered alphabetically.

Last, but not least, many of the menu tree data getter/processing functions I'd consider a hack, resp. very custom. We actually want to move http://drupal.org/project/menu_block into core to remove these from this code.

Posting an idea here on how one might work with shortcuts in the new shortcut bar...

http://blip.tv/file/2304921

Updated patch based on feedback from @Dries and @sun. Actually, @Dries reviewed the longer patch (and said it is quite simple), while @sun reviewed the shorter one. Based on encouragement from Dries, I've been working on the longer one.

Every feedback from Dries was acted up on, except (7). Dries, that code adds body classes to the document based on the current configuration state.

Many of @sun's feedbacks were addressed except these:

+    $page['page_top']['admin_toolbar'] = admin_toolbar();

That function doesn't sound like a function that builds/renders/outputs something.

Well, it does not need to do it itself. It build an array, which is then rendered later by the page rendering. The #theme callback function is added. As far as I am informed, it is the current practice to generate arrays which can be altered and themed later, not to generate straight HTML at every occasion anymore.

+body.admin-toolbar {
+  padding-top: 30px;
+}

That will break any absolute or fixed positioned elements in a theme.

Great, please suggest a better solution for us.

+      $links['menu-'. $item['link']['mlid'] . $class] = $l;

The generated classes from the $links array won't be very useful. Since you process each item anyway, I'd suggest to use a part of the link path instead. However, also keep in mind that modern browsers can use modern CSS to apply styles to [href=xyz].

Not sure why they would not be useful. The menu-$mlid type of classing is what menus do by default, so we do it for consistency. Beyond that all items will have their nice path based classes, just as you suggest, already in the code via the $class variable. We can keep modern browsers in mind, but unfortunately cannot depend on them.

+  color: #ccc;
+
+  position: fixed;
+  left: 0px;
+  right: 0px;
+  top: 0px;
+
+  z-index: 100;

Pointless blank lines. (and elsewhere)

While I don't agree that they were pointless (even this example shows they separate logical groups of properties), I've removed them for the sake of rolling forward.

+div#admin-toolbar div.admin-menu #admin-toolbar-user {
+  position: absolute;
+  right: 10px;
+}
+
+div#admin-toolbar div.admin-menu #admin-toolbar-menu {
+  position: absolute;
+  left: 10px;
+}

Those menus will most probably overlap/clash when not browsing with a maximized window.

"Maximized" is a pretty relative size. The point was made by webchick above. What's your suggestion?

In general, all CSS should follow our CSS coding-standards; specifically: styles/rules should be ordered alphabetically.

I've looked up this CSS coding standards, and it says it is a working document and being discussed. (http://drupal.org/node/302199) I've not heard that added CSS should have all properties ordered alphabetically, and I believe that would further reduce the clarity that was already somewhat reduced by removing those "pointless" blank lines (see above). I don't agree with this suggestion.

Last, but not least, many of the menu tree data getter/processing functions I'd consider a hack, resp. very custom. We actually want to move http://drupal.org/project/menu_block into core to remove these from this code.

Well, other modules including book module in core have their own custom handlers because the menu API is not too rich (to say the least). Let's not postpone this issue on that, but feel free to push efforts to clean this area of Drupal up. I'd be a great supporter!

New patch and new image attached. Still requires the page_top region patch! (See above).

Actually, also did not act on @Dries' renaming suggestion from 'in-overlay' to 'overlay'. I think the link is not *the* overlay, so putting in this direct class on it is semantically not current. We might want to use (and will most probably use) the "overlay" class on the admin overlay itself, so I'd not use it here. Maybe 'overlay-link' or something would be better on the link is 'in-overlay' sounds too non-standard :)

Can we add drupal_alters to the user and admin menus?

@dmitrig01: The admin menu is already coming from the children of the /admin menu item, so whatever you want to do there you'd do by altering the menu items themselves. That would get you consistent behavior across all the presentations of the admin menu, not only this one. Regarding the user menu, I am not sure an independent alter hook is deserved here. Since it is promptly passed on to theming and is not rendered before the actual page is rendered, you have plenty of opportunity to alter it via preprocessors. (Same applies to the admin menu, but that I'd not say would be a good practice to use a preprocessor; also same applies to the shortcuts, but that has some todos which are still need to be fixed so it becomes even more dependent on menus).

---- 1

+name = Admininstration toolbar

Typo of Administration

---- 2

"Maximized" is a pretty relative size. The point was made by webchick above. What's your suggestion?

At the very least we should add a background colour to the overlapping items so that we can clearly read the items on top.

However, it would probably be better to start a new line of links when they collide. I'm not sure how to do that and still account for the appropriate body padding (except via JavaScript), but that may be an appropriate compromise for the edge case. So float #toolbar-user to the right, relatively position #toolbar-menu and give .admin-menu a min-height.

---- 3

+ $id = str_replace('/', '-', $item['link']['href']);

That does not play nice if you happen to have created a menu item to <front>

---- 4

+ $links['menu-'. $item['link']['mlid'] . $class] = $l;

Missing space on the concatenation.

---- 5

+ $l = $item['link']['localized_options'];

$link would be more readable than $l

---- 6

I don't like seeing CSS hacks, but I don't think we have a clean way of implementing conditional comments; which would be my preference.

---- 7
+ * Initialize cautiously to avoid collisions with other modules (admin_menu).

(admin_menu> feels superfluous and possibly confusing.

Discussed the top level menu links today with yoroy, leisa and Bojhan in #drupal-usability, I think we can simplify what's going here a bit based on that discussion and the video yoroy posted / http://www.d7ux.org/d7ux-information-architecture-a-detailed-view/

The idea is that 'Content', 'Appearance', 'Structure' etc. are links directly to common tasks, and 'Config and Modules' is our current /admin page - except that we add more top level categories to /admin (including recently introduced ones like 'international' and 'development' and brand new ones like 'media').

Since the idea is to get the header in quickly, then implement the new IA, by taking that top level of the menu we'll actually make things harder - as international, development etc. will show up top. Instead I think the top level should be a custom menu same as shortcuts - and we menu_link_save() what we need there rather than doing menu system trickery. That makes it just an extra bit in hook_install()

More general review:

First level menus
Second level menus
Would probably make sense to call this toolbar/shortcuts or whatever they're called elsewhere, these comments still suggest hierarchy.

toolbar_enable()
Why not just put all of this in hook_install()? Any reason we need to re-add links if the module is disabled and re-enabled?

+  // Add logout & user account links
+  $output['user_menu'] = array(

If we add these, then we can drop the current user menu in core which was added a couple of months back and put those links back in navigation. Could be a followup patch though.

All the menu stuff should really be in menu.inc if it's needed but there's already a @TODO for that.

+      $l['title'] = '<span class="icon"></span>' . $item['link']['title'];

No icons in the patch, so no need to add placeholders is there?

The rendering code here needs a refresher for #455844: Allow more granular theming of drupal_render'ed elements and #373201: Enhance drupal_render() themeing. Return renderable array on tracker page. (landed a few minutes ago). I will submit an improved patch when gabor says that he is stopping work for a day (or gabor can just make these changes). toolbar_generate() should be named toolbar_build() for parallel with node_build(). in there, we should use $build['foo'] instead of $output['foo']. just a var name change ... template_preprocess_toolbar() is mostly not needed. maybe just the collapsed variable. the other three variables should not exist and instead we should call render($content[''user_menu']) in toolbar.tpl.php. See node.tpl.php for examples of this technique.

DBTNG code style looks wrong in .install

do we need the menu_rebuild() and clear cache in hook_enable()? the modules page already kills us with several of those.

I'm not real pleased yet with the functionality in the patch but the code is pretty small and happy.

Updated patch solving some of the notes which came up since the last review.

@alpritt: 1, 3, 4, 5 and 7 taken care of. Although I don't think front would be a viable menu item in the admin menu, I've added code to remove < and >. On 6, we agree and on 2, I'd love to see actual working code.

@catch: Using or not using a separate static menu for the admin menu part of the toolbar was discussed a few times, but probably poorly documented so far. The final Drupal 7 is planned to ship with the admin menu exposed via the admin toolbar, so adding in a static menu would only be a temporary measure. However people like Dries said it is a good temporary measure to indeed expose these things in the menu, so we can get them fixed quicker. If we keep hiding the ugly, it is not going to get fixed soon.

@catch: CSS comments and hook_enable to hook_install are fixed. The icon placeholder is there exactly to style an icon-like area, so people understand we have something planned for there. See screenshots above. Also, I agree that we should not discuss the fate of the Drupal user menu in this issue.

@moshe: Renamed to _build and $build; I'm not sure how should I formulate a render() structure for theme_links, so that it gets the extra attribute as well, so would welcome your help on that. Looking at the node.tpl.php code, it does indeed change a lot since I last looked at it. So a helping hand in how should that structure to render look like would be helpful.

@moshe: I looked at those menu rebuild and cache issues. The menu_link_save() call does call menu rebuild on the exact menu, which stops having an effect after its second invocation, so we get the most out of it already (with three toolbar items). So I've removed the menu cache clear all. Not exactly sure why did yhahn added the menu_rebuild(), but noting that this will most probably run more in the install process (being part of the default install profile I guess), so looking at what kind of actions are done on the module page is late for the module.

Following up on that, this should also include enabling the module by default, so I've added a hunk to enable it in the default install profile.

What else?

@gabor. Look at he render structure for node links. Be sure to use very latest HEAD ... One easy way is to see it is to enable the $page printing feature of devel module.

It looks like a $build['collapsed'] should be #collapsed.

Would be good toolbar.module had a low system.weight so that modules could alter it easily in hook_page_alter. Like we did for block.module.

@moshe: Are you suggesting defining a one-off element just for this tool, so we can pass on extra classes? Like the #type => 'node_links' element is defined in node module?

/**                                                                                                                    
 * Implement hook_elements().                                                                                          
 */                                                                                                                    
function node_elements() {                                                                                             
  $type['node_links'] = array('#theme' => 'node_links');                                                               
                                                                                                                       
  return $type;                                                                                                        
}                                                                                                                      
                                                                                                                       
/**                                                                                                                    
 * Format a set of node links.                                                                                         
 *                                                                                                                     
 * @param $element                                                                                                     
 *   An associative array containing the properties of the element.                                                    
 *   Properties used:  value                                                                                           
 * @return                                                                                                             
 *   A themed HTML string representing the links.                                                                      
 *                                                                                                                     
 * @ingroup themeable                                                                                                  
 */                                                                                                                    
function theme_node_links($element) {                                                                                  
  return theme('links', $element['#value'],  array('class' => 'links inline'));                                        
}

That sounds like quite some custom code to work around rendering not being able to pass on the right theme parameters. Then we'd need to do something like this?

function toolbar_elements() {                                                                                             
  $type['toolbar_links'] = array('#theme' => 'toolbar_links');                                                               
  return $type;                                                                                                        
}                                                                                                                      
                                                                                                                       
function theme_toolbar_links($element) {                                                                                  
  // #toolbar_class would then be a custom defined rendering API array key here, so we can pass on the right class. 
  return theme('links', $element['#value'],  array('class' => $element['#toolbar_class']));                                        
}

This is what you mean? So then we can use #type => 'toolbar_links' and #toolbar_class => 'admin-shortcuts' for example on the shortcuts? This looks like quite some extra code to me (not to say how custom it is) to keep this unrendered up until the toolbar template is hit, but if this is the way it should be done, then so be it. Please advise.

Updated patch based on feedback from Moshe. Uses more #theme constructs with the style of the latest core code, as referenced by Moshe. Also, got rid of the "collapsed" handling in PHP, since it was hardwired at all times to FALSE. Plus standardized on classes with the toolbar prefix.

I sincerely hope there will be no other systemic changes, before this patch is committed, which would require rerolling considerable parts of the patch. Fingers crossed.

I'm really sorry, I stupidly didn't realize that other patch would have an effect on this one. :(

almost there.

toolbar_preprocess_page() creates a class but that class is not used.

could we get some hook_help() explaining how to customize the links?

color module support would be nice candy.

@moshe:

1. toolbar_preprocess_page() *adds to* the $vars['classes_array'] array, which then ends up in the $classes variable (and $classes_array variable) in page.tpl.php. A page.tpl.php outputs this as classes on the body tag. Not the task of this module to handle that.

2. There is already a TODO to make it role-dependent, so I don't think we should build help text for the current temporary situation.

3. Let's not hold up this for nice candy. Whoever wants to add that on later, go and do it *after* the patch is committed.

Anything else?

Discussed the top-level menu issue from #137/#139 with Gabor and for the record I now agree with the current patch taking just the top level. Patch for initial config/modules page is over at #508458: Config and modules page.

How would role dependant actually work considering that menu system already has a notion of access callback and customizing?

Coming to the party late, I am fully sharing webchick concerns over secondary menu, wait, shortcuts + confusing "edit mode" button. We need to run user tests, sure, but I will eat my old hat if those 2 issues do not raise problems.

Below is my proposal for simplified toolbar: essentially putting all links to 1 line, separating "most common actions", "rest of admin IA" and "other stuff"

Current patch can go in, but later on we need to work hard to test it and get the details right.

Just a reminder this patch is still currently dependent on #468534: Add a region at the top of the page above the header region. as far as I know, which is not RTBC yet.

yoroy rightly marked this postponed in #25 but it was moved back for some reason, the other patch at least needs to be committed along side this so it'll work in HEAD, whether we refactor to hook_footer() or something else later once it's in.

hey Kika

I'm sure you'll agree that the worst thing we can do now is to get into a 'i think this, i think that' debate over the design.
We are really looking forward to getting this out to do some extensive testing and see what needs to be tweaked (and we're the first to say that there will probably be some tweaking required).

Having said that - this design has not evolved in isolation - there has been quite a bit of testing done, albeit limited by the lack of an extensive working(ish) prototype, and we have found that on the whole, people either understood on their own or very quickly learned how the header worked, so I do have reason to be confident that you may yet eat your old hat ;) We shall see.

But shall we focus on getting something into a workable prototype that we can all take out and test intensively and so that we can make decisions based on data not opinion (however expert).

subscribe

damn, I look away for 2 secs and it's pretty much done...

I'm gonna test the hell out of this, especially the fallbacks for smaller resolutions :)
awesomeness!

Short DBTNG review:

+  $query = db_insert('menu_custom')->fields(
+    array('menu_name', 'title', 'description')
+  );
+  $query->values(array(
+    'menu_name' => 'admin_shortcuts',
+    'title' => $t('Administration shortcuts'),
+    'description' => $t('The <em>Admininstration shortcuts</em> menu contains commonly used links for administrative tasks.')
+  ))->execute();

a) It is not necessary to split up between fields and values unless there are multiple values. just remove the first part and rename values to fields.
b) each method call needs to be on a new line.

Something like this:

   db_insert('menu_custom')
      ->fields(
        'menu_name' => 'admin_shortcuts',
        'title' => $t('Administration shortcuts'),
        'description' => $t('The <em>Admininstration shortcuts</em> menu contains commonly used links for administrative tasks.')
      ))
      ->execute();

I agree that we really need to just get this thing in in its current state and then refine later. We can't hope to guess exactly how users will find it confusing until we see it in action, and that's much, much easier to tell once it's part of the drupal-7.x.tar.gz file and not "Oh, well go to this issue, apply the patch, find these images and stick them here, oh and don't forget about the other patch in the other issue..." ;)

So I would really like to commit this today, or else this weekend.

However, in reading through this for the final time, there are a couple of kind of odd things.

+function toolbar_install() {

This entire function confuses me. I don't understand why this isn't hook_menu() in toolbar.module. If the idea is you want to control the destination parent menu of these links rather than shoving them into "Navigation" then use the 'menu_name' key like Devel module does:

  $items['devel/reinstall'] = array(
    'title' => 'Reinstall modules',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('devel_reinstall'),
    'description' => 'Run hook_uninstall() and then hook_install() for a given module.',
    'access arguments' => array('access devel information'),
    'menu_name' => 'devel',
  );

If there is a particular reason why this needs to go in hook_install(), (Also can we use API functions for any of this stuff? Raw DB queries make baby kittens cry.) then we need a corresponding hook_uninstall() so this module remains optional. It might make sense to move these to hook_enable()/hook_disable() if that's the case.

+/**
+ * Implementation of hook_page_alter().
+ * 
+ * Add admin toolbar to the page_top region automatically.
+ */
+function toolbar_page_alter(&$page) {
+  if (user_access('access toolbar')) {
+    $page['page_top']['toolbar'] = toolbar_build();
+  }
+}

I'm really, really not happy with this as a long-term solution, as it forces all themes to have a 'page_top' region, even if in the custom theme for Drupalcon Paris it might be reasonably called the 'haut_de_page' region. I'm not going to hold up committing on this because it's something we can improve later, but can you explain why this was ever seen as a viable solution? I'd like to know if I'm missing a clue bat.

+function toolbar_build() {
...
+  // Add logout & user account links
+  $build['toolbar_user'] = array(
+    '#theme' => 'links',
+    '#links' => array(
+      'account' => array(
+        'title' => t('Hello <strong>@username</strong>', array('@username' => $user->name)),
+        'href' => 'user',
+        'html' => TRUE,
+      ),
+      'logout' => array(
+        'title' => t('Logout'),
+        'href' => 'user/logout',
+      ),
+    ),
+    '#attributes' => array('id' => 'toolbar-user'),
+  );

Just a question: why are the user links hard-coded and not taken out of the User links menu where we've already split them out? We could simply take the "My account" link and give it a title callback that does Hello, <strong>@username</strong>, no?

+function toolbar_get_menu_tree() {
...
+    if ($item['link']['link_path'] == 'admin' && !empty($item['below'])) {

Where is $item['below'] ever set? I grep in the patch and I only see references within this function. Is this a leftover from earlier in the patch?

Then just silly, minor stuff I saw as I glanced through...

+  font: normal 11px/20px "Lucida Grande",Verdana,sans-serif;

Add spaces between font names here.

+  foreach ($tree as $k => $item) {
...
+  foreach ($tree as $k => $item) {

Change $k to $key

+    // Set the intial state of the toolbar
...
+  // Gather active paths

Add a period here.

@Berdir: Ok, put new method calls on their own line. I don't think you were serious about 3 space indentation in your example, so I only used 2 space indentation. Also implemented your fields() suggestion.

@webchick (and also @moshe): The menu items are not a hook_menu(), because they are "custom" shortcuts. Users will mess around with them. If only that would happen, hook_menu() would probably be better (users could reset their items), but this way, install profiles can set their own shortcuts. Even better, on the plan is to have per-role menus set up, so depending on your role, you'd have different sets of shortcuts (eg. moderators vs. translators). Obviously you can have more roles then one and a UI needs to be added for this and so on. So we can hold this patch up by either refining the current temporary approach and not let it be tested, or waiting on design and discussion on the inner workings of an improvement of this patch. Either way, this issue is a month old, and this rate, we are getting at most one more d7ux improvement in, since code freeze is less then two months from now, so maybe we should not obsess ourselves with temporary details. Obviously if we drag this issue on for long enough, these details will not be temporary anymore, so in that case, we do need to have a refined solution here. Not sure the module should remove a menu on uninstall, which might have been altered and used elsewhere in an install profile. It is on purpose set up by the module, but might not only be used by the module.

@webchick: On the page_top region, I think you were also on the other region discussing, eg. when we made the 'content' region mandatory. There, we discussed, that themers live in an English dominated environment. They get their $classes, $odd, $even, etc. The internal name of regions end up in variable names, while nice, human facing names can be added to regions as well. French themes would add nice human facing names in French. Obviously they can use French region names, in which case, all their other variables generated by Drupal core will be English, with a few French. I am not sure people go into the trouble and give themselves a hard time like that. I did not inspect themes built in exclusively foreign language environments, so I can still be wrong, but I think people would avoid this trouble regardless of page_top or content being required as-is.

@webchick: On the user menu, that is not an *admin user* menu. If we make it an admin user menu (reuse it in the admin header), then if a website puts in "your friends", "your account balance", etc. into its user menu, those items will nicely show up on top in the menu too. Since we have a very limited space and very limited scope for that part of the admin menu, if we gonna use a concrete menu for it, we should name it so that is is evident that is the admin user menu, and is not to be played with (too much). In that case, we probably need to have a normal user menu too, so that those who don't see the admin header can still see their account page and log out.

@webchick: $item['below'] is how the menu system gives you all items below the current menu item in a huge bad-ass nested structure. There is no API function whatsoever to get children of a menu item, so we had two choices: either query the tables directly (and do our own localization and access checking duplicating quite a big chunk of menu functionality) or use the API available, which is basically either getting *all* menu items or getting childrens of the item which corresponds to the current page. Since the later is not viable for a permanent menu, we need to use the other way. (Discussed this with pwolanin when searching for a better way). Since the whole tree is cached, we get it quickly. We still might want to cache our own version of the menu once we get through the toolbar_menu_navigation_links() function, if you think that would be a good idea here. That would be a new cache of the admin menu per user.

Attached patch also skips enabling the management menu by default, since that is covered in the admin toolbar.

The hook_menu() comment by webchick isn't about the shortcuts themselves, but about the overall menu - you can give it a menu name, then menu_link_save() the rest of the items with that as one of the array keys. But let's discuss how best to add these menus over at #473082: Add custom menu API. I also just opened #509584: APi fixes for menu_tree_data() including depth param to see if we can clean up the 'below' stuff (in a followup issue again).

catch is correct about where I was advocating the use of hook_menu(). I think it would make the code slightly cleaner. But there's probably a lot in this patch that could potentially be made slightly cleaner, and it will be easier to figure out what those are and clean them up once this patch lands so we're not juggling stuff so much.

I was not overly involved in the content block patch, but I guess I can see the language argument.... I'm mostly concerned on a technical level though that Admin Menu abandoned that approach long ago, as well as the fact that while "content" is semantic, "page_top" is not. But again, something we can clean up after.

Fair enough on the rest.

I'm marking RTBC, to give people a chance for last-minute feedback. Otherwise, I will commit tomorrow unless Dries beats me to it.

I reviewed the patch once more and committed it to CVS HEAD!!! Oh my. :)

Thanks to all the people that helped, with special kudos to Gábor Hojtsy, yhahn, markboulton and leisareichelt. This is a big deal for Drupal 7 and will hopefully mark the start of many more subsequent usability improvements.

In fact, we need to add a CHANGELOG.txt entry here.

Awesome! Let the breakout issues begin. I truly think this will be a boon to Drupal's adoption by less programmer-oriented web developers, and will keep Drupal at the top of my list of recommended blogging/site building platforms.

The toolbar overlaps/hides sticky table headers.

#510058: toolbar.module not enabled by default

OK here's all the issues which need to fixed to undo the various hacks and broken things committed with this one. I would open issues for all these, but there's too many and I'm on my way out.

The @TODOs for the various toolbar_menu_* functions. I suggest either a depth parameter to menu_tree_all_data() or a new API function for getting slices of a menu.

#510058: toolbar.module not enabled by default linked above.

We should revert #468534: Add a region at the top of the page above the header region. and add the toolbar in hook_footer(). The motivation behind adding a region was to make the toolbar header a 'real block which you can move around', however this patch uses hook_page_alter() to add the block to the header, which defeats that purpose entirely, so it should just be done in hook_footer() and use the same mechanism as admin_menu.module does now for positioning.

On admin/build/modules the header has extremely bad jumping behaviour when scrolling down the table.

On admin/user/permissions sticky table headers are broken (pointed out by sun above).

We should remove the placeholders for icons, and then add icons in when we actually have some.

Add a changelog.txt entry.

There may well be more but that covers everything which had been brought up earlier in the issue and deferred until after commit, or which I could find from five minutes trying out the new header with HEAD.

Marking needs work - please only mark back to fixed when these followup issues have been properly documented elsewhere.

#510126: Admin toolbar breaks style on Garland when js is disabled.
Adding this to the list of problems to solve.

Just a note the choppy behaviour on scrolling in admin/build/modules can also be seen on content type forms - I think it's probably the presence of collapsed fieldsets.

edit, I'm getting this on firefox 3.0/ubuntu. Couldn't reproduce in Opera.

Posted #510340: Remove icon placeholders from toolbar header..

Unable to install with minimal install profile. See #510368: Should toolbar depend on menu module? (for menu_custom table)

Changelog patch attached.

Also opened #511286: D7UX: Implement customizable shortcuts, #511284: Add html_top and move the admin toolbar into html_top and #511258: Do not enable the management menu by default.

Opened #511926: Toolbar cannot be navigated via a keyboard

A number of posts above raised the issue of how to manage wrapping with narrow viewports. Proposed patch here: #515334: D7UX toolbar: Usability in narrow viewports

#520364: system_admin_menu_block_access() makes no sense

#520368: Admin toolbar breaks sticky table headers

Ok, crossposting the test video here also. http://vimeo.com/groups/drupalux/videos/5657241

Created http://drupal.org/node/524802 as a result: Links and tabs are duplicated in site theme and admin header.

Will pull out more issues from that. One is that users might not find the "meta informations". But this may rather belong to the Admin theme issue.

Ack, how do I create these nice links to issues that take on the color of the actual status of the issue?

@ eigentor: Just post it with brackets [] around the number after the pound sign (#)... so it would go:

#524802: Admin header duplicates tabs and links in theme which translates into --> #524802: Admin header duplicates tabs and links in theme

Edit Link gone?

I wonder if the original concept of the "Edit" Link in the Shortcut bar is still in the game? I remember Gabor saying that this would be hard to implement, since it has to be dynamic and would have to fetch edit Links on the page that might not be implemented in a streamlined way.

Still it would be a wonderful feature to have, and not looking for any edit tabs and links.
Sure this could not make for edit links in tables, but only if there is only a primary one that leads from mymodule/item
to mymodule/item/edit.

@eigentor: see #473268: D7UX: Put edit links on everything

Fabulous :)
Ah, D7, my love...

Not entirely about toolbar.module but adds a cache for toolbar_get_menu_tree() which is taking up 25% of page execution time at the moment.

#519046: Clean up toolbar menu code

subscribing

Edit Link gone 2.0
A bit slow, I have just now realized that "put edit links on everything" is something else than I meant.

So we stepped away from this concept of an edit link?

Edit Link gone 2.0
A bit slow, I have just now realized that "put edit links on everything" is something else than I meant.

So we stepped away from this concept of an edit link?

@eigentor: the edit toolbar button was supposed to do two things (1) let you edit some content inline (2) do all the stuff the edit links on everything patch does. See http://www.d7ux.org/edit-on-page/ and compare mockups to how the edit links on everything patch works. As usual, all the stuff assumed by this edit toggle is too much for a patch, so we can either not implement any of that based on concerns that one patch does not implement everything, or help it get in piece by piece. Last time I checked, there was an edit toggle on the toolbar in the edit links in everything patch. Even though where to put that toggle is still debated (because it is different to other items in the toolbar which are not "mode toggles").

WTF? Why on earth is Toolbar module hi-jacking Drupal JavaScript namespaces?

// $Id: toolbar.js,v 1.13 2010/01/07 07:57:09 webchick Exp $
...
/**
 * Implementation of Drupal.behaviors for admin.
 */
Drupal.behaviors.admin = {
...
/**
 * Initialize cautiously to avoid collisions with other modules.
 */
Drupal.admin = Drupal.admin || {};
Drupal.admin.toolbar = Drupal.admin.toolbar || {};
...
/**
 * Retrieve last saved cookie settings and set up the initial toolbar state.
 */
Drupal.admin.toolbar.init = function() {
...
/**
 * Collapse the admin toolbar.
 */
Drupal.admin.toolbar.collapse = function() {
...
...
...

Created an issue for #181: #686670: Inconsistent code style in toolbar.js

What else needs to be done in this issue?

Nothing