Similar to how http://drupal.org/node/36465 made menu items to the home page work, this patch will give a Primary Link to the home page a class='active' attribute if you're on the home page. This works with all links sent through the l() function.

e.g. if you're on the home page, and you have a primary link to the home page, currently the link doesn't have the active class. With this patch it does. This applies whether or not 'node' is your default home page.

Thanks to colleague 'Guntrisoft' for help with this.

CommentFileSizeAuthor
#10 home_active_link_4.patch562 byteschx
#9 home_active_link_3.patch4.61 KBchx
#8 home_active_link_2.patch784 bytesjakeg
#7 home_active_link_1.patch726 bytesjakeg
#1 home_active_link_0.patch782 bytesjakeg
home_active_link.patch351 bytesjakeg

Comments

jakeg’s picture

jakeg commented
StatusFileSize
new home_active_link_0.patch782 bytes

making a unified patch instead with -up. Thanks to chx for the heads up.

jakeg’s picture

jakeg commented

To test this, add a primary link to (home page) and check the HTML source when you're on the home page. You'll notice there's no class='active' for the link, whilst there is for the other links when on their pages. Then install this patch and try again and you'll see it works as expected.

Bèr Kessels’s picture

Bèr Kessels commented
Status: Needs review » Reviewed & tested by the community

+1. Afte ra lengthy discussion on IRC, jakeg finally managed to let me understand why the is there.
I was a bit hesitant, because of all the checks in that if. They would perform a bit less well, and add quite some cruft.

Anyway, it seems there is no other way, then to 'hardcode' these frontpage checks in l(). IMO it makes a lot of siense, because it fixes an annoying consistancy bug.
My 'tabs' and links wold never highlight for the frontpage.

this patch finally fixes that.

Zen’s picture

Zen commented

I can't think of any alternative either: +1. But can you please add brackets/parantheses around the conditions? I misread it initially.

Thanks :)
-K

jakeg’s picture

jakeg commented

Zen: is it coding standards to add them or not?

Zen’s picture

Zen commented

Yep, though it is not always adhered to everywhere with any degree of consistency. Ref: http://drupal.org/node/540

Either way, they will help in readability in this case - I'll be happy with a pair around the variable_get condition :)

</ nitpick> :)
-K

jakeg’s picture

jakeg commented
StatusFileSize
new home_active_link_1.patch726 bytes

This patch is as above but with an extra set of (brackets) for better readability. Please vote on this very simple but effective one line patch for 4.7.0 release.

jakeg’s picture

jakeg commented
StatusFileSize
new home_active_link_2.patch784 bytes

sorry, forgot to remove old line. Disregard last post, this is the actual patch.

chx’s picture

chx commented
StatusFileSize
new home_active_link_3.patch4.61 KB

Zen misunderstood that handbook page. We very rarely use () where it's not absolutely not necessary.

I like this patch though and rerolled _without_ that ().

chx’s picture

chx commented
StatusFileSize
new home_active_link_4.patch562 bytes

I patched too much.

Zen’s picture

Zen commented

I wasn't only trying to adhere to code conventions here - I just pointed out that I misread the code at first glance due to the lack of brackets :)

Anyways, I'm cool with it :) +1

-K

simon rawson’s picture

simon rawson commented

+1 from me. I would like to see this committed asap.

drewish’s picture

drewish commented

+1 this would make my day.

killes@www.drop.org’s picture

killes@www.drop.org commented

I am not too happy with it l() is called quite often and any change to it has a performance impact. I am not sure it is possible, but maybe we can move this check to the calling function.

jakeg’s picture

jakeg commented

killes: l() already has 7 arguments, lets not add another. I don't see why the functionality on the home page has to be any different from elsewhere on the site. If a link is to a page that it is on, then it should be possible to theme it differently, even on the home page.

if ($path == $_GET['q'] || ($path == '<front>' && variable_get('site_frontpage','node') == $_GET['q'])) {

I've added the code from the || or.

$path == '<front>'

... will be a very quick check, not requiring a db query. Only if that returns true (i.e. on the home page) will the variable_get() function be called. Thus on all pages but the home page, there should be no change in performance. On the home page, there will be the extra time taken for the variable_get to return true/false. That is all.

But, in my opinion, this marginal performance drop on the home page would be worth it for the increase in (expected) functionality.

gerhard killesreiter’s picture

gerhard killesreiter commented
Status: Reviewed & tested by the community » Needs work

I am not convinced, please provide benchmarks.

jakeg’s picture

jakeg commented

Killes: this is the expected functionality, that such links should be class='active' on the home page. I believe this is the most performacne-efficient way of doing this. Yes, it is extra code on the home page, but I don't know of a less performance intensive way of doing this.

Mirrorball’s picture

Mirrorball commented

It's a must.

zach harkey’s picture

zach harkey commented

+1 Agreed, this is a must.

Otherwise we are forced to break Jakob Nielsen's Top Ten Most Violated Homepage Usability Guidelines #10 Don't include an active link to the homepage on the homepage:

Active links to current pages cause three problems:

  • If they click it, a link leading to the current page is an utter waste of users' time.
  • Worse, such links cause users to doubt whether they're really at the location they think they're at.
  • Worst of all, if users do follow these no-op links they'll be confused as to their new location, particularly if the page is scrolled back to the top.

Homepage links on the homepage typically result from using a universal navigation bar that includes "home" as an option. Fine. But when users are on a page that's featured in the navbar, you should turn off that option's link and highlight it in such as way that indicates that it's the current location.

thomaslaw’s picture

thomaslaw commented
Priority: Normal » Critical

This should absolutely be added as an embedded option. Having to create Home links makes my job as a designer harder and leads to extra markup to do just one thing.

jakeg’s picture

jakeg commented
Version: x.y.z » 4.7.0-rc2
Status: Needs work » Needs review

I'm glad its not just me that needs this. My patch works. I'm changing this so its against 4.7 so maybe it gets noticed and acted upon.

jakeg’s picture

jakeg commented
Version: 4.7.0-rc2 » 4.7.0-rc1

rc2 isn't showing in the contributor block item '4.7 critical issues' yet, so changing to rc1

jakeg’s picture

jakeg commented

oops sorry, because this is a 'feature' and not a bug, it won't show up there anyway.

zach harkey’s picture

zach harkey commented
Category: feature » bug
killes@www.drop.org’s picture

killes@www.drop.org commented
Category: bug » feature
Priority: Critical » Normal

I've requested benchmarks, these were not provided.
this is in no way critical and it isn't a bug.

zach harkey’s picture

zach harkey commented

How is this not a bug? It is totally inconsistent with the way all other menu links behave when on the current page, making it very difficult to use the menu module for global/primary navigation where a 'home' link is expected as convention.

The whole point behind the class="active" is so that we can provide the user with a visual indication as to where they are on the site — a convention that is entirely dependent on consistency.

The fact that there is no reasonable way to achieve this consistency on the home page is a bug, not a feature.

What if the module applied class="active" appropriately for every page except for taxonomy nodes? Wouldn't that be a bug? Why should the home page be any different?

Because the bug relates to the single most important page on the whole site (the front page) is what makes it critical - imho.

Will White’s picture

Will White commented

I agree. This is very important. It's a must that a visitor knows where they are at all times using visual cues. Basic usability.

Mirrorball’s picture

Mirrorball commented

Asking for this to be included again.

killes@www.drop.org’s picture

killes@www.drop.org commented
Version: 4.7.0-rc1 » x.y.z

moving

drumm’s picture

drumm
he/him
Location NY, US
commented

I agree this should be fixed, but..

When I added the first instance of <front> to the block module I never expected it to get beyond being a necessary UI convention (because a blank line is indistinguishable from an empty line). How is it getting all the way to l()?

patrickharris’s picture

patrickharris commented

Will this ever be fixed? It means I can't use menu module for primary navigation

dries’s picture

dries commented

The entire front page handling in Drupal is a PITA. Maybe we need to fix it by overwriting $_GET['q'] with '' if we're looking at the front page. Or something. It might simplify a number of front-page checks ...

Will White’s picture

Will White commented
Version: x.y.z » 5.0-rc1
Status: Needs review » Closed (duplicate)

The problem should be fixed with this issue.
http://drupal.org/node/77485

snufkin’s picture

snufkin commented
Status: Closed (duplicate) » Active

This is not the same as active trailing, because active trailing is focused on menu items with child items (submenu).

Quoting from there:

Outputs this special class as a new class called 'active-trail', rather than re-using the existing 'active' class. This allows 'active-trail' links to be styled differently to 'active' links. For example, some themes may want to style it in a different colour to 'active', while other themes may not want it to be styled differently to normal links at all (i.e. they may want it to behave just as it does now).

This issue still has validity, because it changes the way the "active" class is given to items.

You can check a preview image here. When on the "About" the menu item does not receive the "active" class, though it is the selected item.

dpearcefl’s picture

dpearcefl commented
Status: Active » Closed (won't fix)

Considering the age of this issue with no responses and that D5 is unsupported, I am closing this issue.