This is a proof of concept patch in response to the discussion: #410646: "Secondary menu" exists but is no longer the default source for the secondary links

One of the solutions proposed is to remove the secondary menu from core and move the secondary links to the header - so the user menu correctly displays at the top of the theme rather than at the bottom which has been identified as a problem in Bartik (and core aka stark).

This is not a feature request or bug report, its a patch to apply should this be the direction taken and I am simply opening a dialog of how we might do this.

CommentFileSizeAuthor
#26 bartik-move-secondary-links-889982_2.patch9.31 KBJeff Burnz
#18 bartik-secondary-links-in-header_pipes.png52.29 KBJeff Burnz
#18 bartik-secondary-links-in-header_pipes_bottom.png49.74 KBJeff Burnz
#17 bartik-move-secondary-links-889982.patch8.58 KBJeff Burnz
#17 bartik-secondary-links-in-header_2.png19.39 KBJeff Burnz
#1 bartik-secondary-links-in-header.png10.57 KBJeff Burnz
bartik-move-secondary-menu-proof-of-concept.patch2.79 KBJeff Burnz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented
FileSize
bartik-secondary-links-in-header.png10.57 KB

Obligatory screenshot.

bartik-secondary-links-in-header.png

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

I belive I like this...
RTL screenshot?

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Looks great to me. And it moves us along in closing a hard critical. @Jen?

jensimmons’s picture

jensimmons CreditAttribution: jensimmons commented

This is a duplicate of #410646: "Secondary menu" exists but is no longer the default source for the secondary links — the patch here does the same thing as your patch, and also some other things: http://drupal.org/node/410646#comment-3358040

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented
Status: Needs review » Closed (duplicate)

Aye, this patch's work is done ;)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Priority: Normal » Critical
Status: Closed (duplicate) » Needs review
Issue tags: +Usability

Ok. Since #410646: "Secondary menu" exists but is no longer the default source for the secondary links turned into a nightmarish train-wreck of an issue, we decided to vastly simplify the patch there and not deal with the "user links showing up in the exact opposite place from where they should in Bartik" problem, and instead deal with that here since it needs Bartik-y people on it, and not developer-y people on it. :P

I'm also escalating to critical because this is a regression from the UX team's original vision for the user menu which core conformed to for the better part of its entire release cycle before Bartik made it in.

Jeff's screenshot looks great to me, but let's hear what Jen has to say.

jensimmons’s picture

jensimmons CreditAttribution: jensimmons commented
Assigned: Unassigned » jensimmons
Status: Needs review » Needs work

Yes, let's do this here now. No longer a duplicate.

There are two steps:
1) Move the XHTML in Drupal so that the secondary links is right after the main menu/primary links in the source order. (Yes? We aren't doing content first, so...) — also, make sure the semantics of the markup make sense.
and
2) Write CSS so it looks good.

As for the look — what Jeff's patch does is cool:

Only local images are allowed.

but I think it would look better with a pipe.

Also — we should be sure to include these links in the color module support, so that they change colors with the header text color. It's FINE for it to be rolled together with the main title link color. In the screenshot, both are white. At times the title link is black (in other color schemes) — this menu's links in that case should also be black. Or whatever color the user has chosen.

So this needs work. And also, more review. I'll look at the code of the above patch more tomorrow.

jensimmons’s picture

jensimmons CreditAttribution: jensimmons commented
Title: Move secondary links to the header - proof of concept » Move secondary links to the header in Bartik

.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

Subscribing.

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented

My patch fails the requirement in one minor way, the source order. It sticks the actual code at the top of the source order - the alternatives are not many here peeps - if we really do want the secondary links below the main menu links in the source order then we need some tricky positioning magic to do so - because we must account for header region permutations - so we need something very solid and reliable.

jensimmons’s picture

jensimmons CreditAttribution: jensimmons commented

Yes — we need to absolute position this, in a relatively positioned container. Negative margins won't work, since we don't control the height of the header. It shouldn't be to hard.

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented

Ok, lets get the last code issue fixed and commit this.

BenK’s picture

BenK CreditAttribution: BenK commented

Subscribing...

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented

Yes, either position: absolute; or remove the nav wrapper and use some tricky floats - I think probably position:absolute; is going to be way to go, and we set a margin top or similar on the header region.

woeldiche’s picture

woeldiche CreditAttribution: woeldiche commented

Subscribe. Position absolute seems the most robust way to do content-first.

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented

DISCLAIMER: Next one after this reply, can't agree unless he/she has a patch.

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented
Status: Needs work » Needs review
FileSize
bartik-secondary-links-in-header_2.png19.39 KB
bartik-move-secondary-links-889982.patch8.58 KB

Please note that this ties in really close with #845834: Fix Bartiks Header because its totally borked - so the approach taken here has to conscious of what we can do there also.

What this patch does:

- fixes critical hard of moving the secondary links to the header, below the main-menu links in the source order
- places both menus in their own wrappers so we have flexibility with regards to layout method (you could use floats...)
- initially I have used position: absolute, with position: relative on the #header .section wrapper
- made the secondary anchors inherit the color settings for title/slogan
- removed the hard coded header anchor hover colors - this is not going to work being hard coded to gray.

Things to think about...
- the anchor link colors - not just because we now have a menu up there but also in relation to #845834
- position: absolute? Are we serious?
- #845834 and how this ties in
- needs RTL love

Please do not RTBC this without some serious testing, screen-shots, browser testing and feedback from the maintainers and strong consideration for the header region positioning, last thing I want is to come back and fix this later.

The patch is not perfect (theres some whitespace probably...ekkk) so lets just get the approach down and worry about the little patch details later.

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented
FileSize
bartik-secondary-links-in-header_pipes_bottom.png49.74 KB
bartik-secondary-links-in-header_pipes.png52.29 KB

Couple of variants - both have pipe separators which Jen asked for and I forgot in the patch above:

BarisW’s picture

BarisW CreditAttribution: BarisW commented

I'd prefer the bottom variant, with white text by default. Black on red is very hard te read.
Currently I'm sprinting on a Windows laptop of the Drupal Association, which makes it hard for me te apply the path.

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented

BarisW - the link color is the same as the site name and slogan which is set in the color module settings for the theme - so it changes depending on user choice.

tizzo’s picture

tizzo CreditAttribution: tizzo commented
Assigned: jensimmons » Unassigned
Status: Needs review » Reviewed & tested by the community

This looks good, marking RTBC.

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented
Status: Reviewed & tested by the community » Needs review

Jeez, read the comments please. Jeff Burnz explicitly asked not to mark this RTBC without feedback.

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented
Issue tags: +Needs design review

Umm, yeah, needs design review from Jen, Bojhan, yoroy ect first before we even think about committing this. For one thing Bohjan has expressed concern over the similarities with d.o redesign in past issues and this could be taking this even closer... The problem is however, if we don't do this, then what do we do?

The above example that shows the User menu at the bottom of the header is actually very fragile, it would sit on top of the Main Menu tabs if they extended far enough across - I really just included that one to show that a user could make that change themselves with something as simple as "bottom: 0;".

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

The secondary links at the top right has my vote. Having it at the bottom of the header allows for less links in the main menu.

Also it just looks tighter. People expect it at the top right, not next to the main navigation.

Drupal 7 is really looking nice from what I have seen so far.

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented

Are jensimmons concerns adressed? If so I am oke with the design changes of this patch, I dont think it creates to much similarity with the d.o redesign as that clearly has a top navigation bar.

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented
FileSize
bartik-move-secondary-links-889982_2.patch9.31 KB

This is updated patch to include RTL support and cleans up a white space in the previous patch.

The patch does use position absolute which is really the only way I can see that we can achieve both the source order requirement and position the links in the top corner (Jen concurs this the right way to go).

I've tested this is IE6, 7, 8, Firefox 3.6, Opera 10 and Chrome 5.

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented
Status: Needs review » Reviewed & tested by the community

Lets mark this RTBC(hope it gets more attetion now), clearly there havn't been any reviews rejecting jeff's patch and we want to get D7 out the door.

yoroy’s picture

yoroy CreditAttribution: yoroy commented

What this patch does:

Screen shot 2010-09-09 at 1.30.21 PM.png

Jen called for a pipe: " | " inbetween the items, and it was there in a couple versions of the patch but not in the last few. I'm happy to *not* have the pipe seperating items. It's not necessary, not many items will be added to this menu and its likely to fail/look weird in some color choices for the header background.

Any further tweaking would be minor and hold up the important actual fix of getting these links from bottom left to top right where they belong.
RTBC indeed.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Excellent!

Committed to HEAD. Thanks, folks!

jensimmons’s picture

jensimmons CreditAttribution: jensimmons commented

Yeah!

I've been so, so sick with the DrupalFlu I haven't been able to look at Bartik at all. So I'm glad other people pushed this through. :D

Status: Fixed » Closed (fixed)
Issue tags: -Usability, -Needs design review

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +Usability, +Needs design review

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.