as reported at http://drupal.org/node/139015, in D6, breadcrumbs include the current page. we need to peel off 1 level of the menu tree at the right moment when generate the breadcrumb trail.

CommentFileSizeAuthor
#9 last_breadcrumbs.patch_2.txt2.68 KBtheborg
#7 last_breadcrumbs.patch_1.txt2.57 KBdww
#4 last_breadcrumbs.patch2.49 KBtheborg

Comments

chx’s picture

chx commented

This in itself is not a biggie, but I will only fix when the multiple menus are done, I do not want to break menu builder while Peter is in it.

theborg’s picture

theborg commented

Why don't just make an option?. Maybe in theme settings: Toggle Display: Current page breadcrumb.

I've been patching 5.1 core to get the current page breadcrumb trail just like firefox site.

dww’s picture

dww
we/he/they
commented

yeah, i agree that making this flexible would be cool.

theborg’s picture

theborg commented
StatusFileSize
new last_breadcrumbs.patch2.49 KB

This patch modifies:
menu.inc,
theme.inc,
system.module.

It includes an option on theme settings to toggle display of the last breadcrumb trail (or current breadcrumb).

moshe weitzman’s picture

moshe weitzman commented

instead of the terse t('Last breadcrumb') text, we could use t('Current page displays in breadcrumb')

dww’s picture

dww
we/he/they
commented
Category: bug » feature
Status: Active » Needs work

i didn't realize this, but other menu changes that went in had the side effect of fixing the original bug i reported. so, this issue is now just a feature request to make this behavior configurable (which would still be cool).

moshe is right: t('Current page displays in breadcrumb') would be a lot better.

however, this patch needs work for other reasons, too. i just tried it out, and the behavior of the breadcrumb trail is very inconsistent on admin pages that involve tabs. something about the logic in there is wrong. for example, with the patch applied and the new setting enabled, visit:

?q=admin/build/path
Home › Administer › Site building › URL aliases

so far, so good.

now, click on the "List" tab (the default), which sends you to:
?q=admin/build/path/list
Home › Administer › Site building

huh? i'd expect "URL aliases" to be there, since that's the current page.

then, click on the "Add alias" tab:
?q=admin/build/path/add
Home › Administer › Site building › URL aliases

that's closer, but "URL aliases" in this case brings you to the "parent" page, not the current page, and it seems like the goal of this feature is to make it so that the final thing in the breadcrumb always brings you where you are.

perhaps the goal of this feature isn't well thought-out regarding pages with tabs and subtabs, but certainly the ?q=admin/build/path/list case needs to be fixed to put "URL aliases" in there correctly.

also, minor code style issue -- as the last entry in each array, both of these lines should have a trailing comma:

+      'last_breadcrumb'
+    'last_breadcrumb'      => t('Last breadcrumb')

just because the code already there is wrong doesn't mean this patch should perpetuate the error. ;) thanks.

dww’s picture

dww
we/he/they
commented
StatusFileSize
new last_breadcrumbs.patch_1.txt2.57 KB

i took a brief look at the logic, and am confused by what menu_get_active_trail() is returning in the various cases of tabs and subtabs i outlined in the above comment. :( i don't really have time to dive into this deeply, so'll leave this in the hands of someone who cares more. however, here's a reroll that at least changes the setting name and fixes the code style issues. ;)

theborg’s picture

theborg commented

Thanks for the corrections, I'm looking for a resolution.

theborg’s picture

theborg commented
StatusFileSize
new last_breadcrumbs.patch_2.txt2.68 KB

This patch modifies:
menu.inc,
theme.inc,
system.module.

Updated variable name to: current_breadcrumb.
Changed menu_get_active_breadcrumb to display current breadcrumb always in tabs.

pwolanin’s picture

pwolanin commented

The logic involved in building the tabs is a bit tricky, and it a bit divergent in terms of breadcrumb vs. tabs on the page.

For the breadcrumb, we put callbacks in the {menu_links} as hidden links, so that they are in the appropriate place in the menu links hierarchy. However, this only works or paths with no variable arguments and it cannot work for paths like node/%, since these can appear many places in the menu.

The patch looks like a reasonable start if this is a desirable feature. As you can see form the logic, it's a bit tricky to determine when to NOT show the last link (array_pop).

Also, this code could use work:

$breadcrumb[sizeof($breadcrumb)-1] = '<span>' . $end['title'] . '</span>';

among other things, probably the latter part should be a theme function, and should associate some class with the span.

theborg’s picture

theborg commented

the latter part should be a theme function

function theme_breadcrumb($breadcrumb) {
  if (!empty($breadcrumb)) {
    return '<div class="breadcrumb">'. implode(' » ', $breadcrumb) .'</div>';
  }
}

I didn't include a class because the span tag can be styled within breadcrumb divisor: div.breadcrumb span {font-weight: bold}, but maybe it woud be easy to have one.

chx’s picture

chx commented
Status: Needs work » Fixed

Dunno what's the correct status for this, other issues dealt / deal with breadcrumbs, so it's eihter fixed or duplicate.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)

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