#7418 closed defect (fixed)
[PATCH] Active navigation items are broken in Clearsilver templates
| Reported by: | anonymous | Owned by: | Christian Boos |
|---|---|---|---|
| Priority: | low | Milestone: | 0.12.3 |
| Component: | general | Version: | 0.11-stable |
| Severity: | normal | Keywords: | clearsilver navigation |
| Cc: | Branch: | ||
| Release Notes: | |||
| API Changes: | |||
| Internal Changes: | |||
Description
It looks like changes made to move templates away from Clearsilver have introduced a regression where active navigation items are not highlighted properly.
To reproduce, use a plugin that returns a Clearsilver template from its process_request() method. You will see that the "mainnav" navigation bar does not render the active item highlighted.
The bug is that the CSS class "active" is not assigned to the list element in the navigation bar.
The fix is to change trac/web/chrome.py:
class Chrome(component)
...
def populate_hdf(self, req):
...
for category, items in req.chrome['nav'].items():
+ req.hdf['chrome.nav.%s' % category] = items
- for item in items:
- prefix = 'chrome.nav.%s.%s' % (category, item['name'])
- req.hdf[prefix] = item['label']
And then change trac/templates/header.cs:
<?cs def:nav(items) ?><?cs
if:len(items) ?><ul><?cs
set:idx = 0 ?><?cs
set:max = len(items) - 1 ?><?cs
each:item = items ?><?cs
set:first = idx == 0 ?><?cs
set:last = idx == max ?><li<?cs
if:first || last || item.active ?> class="<?cs
if:item.active ?>active<?cs /if ?><?cs
if:item.active && (first || last) ?> <?cs /if ?><?cs
if:first ?>first<?cs /if ?><?cs
if:(item.active || first) && last ?> <?cs /if ?><?cs
if:last ?>last<?cs /if ?>"<?cs
+ /if ?>><?cs var:item.label ?></li><?cs
- /if ?>><?cs var:item?></li><?cs
set:idx = idx + 1 ?><?cs
/each ?></ul><?cs
/if ?><?cs
/def ?>
Attachments (1)
Change History (12)
comment:1 by , 16 years ago
| Milestone: | → 0.11.2 |
|---|
comment:2 by , 16 years ago
| Keywords: | clearsilver navigation added |
|---|---|
| Milestone: | 0.11.2 → 0.11.3 |
| Priority: | normal → low |
by , 15 years ago
| Attachment: | active_nativigation_fix.diff added |
|---|
comment:3 by , 15 years ago
| Summary: | Active navigation items are broken in Clearsilver templates → [PATCH] Active navigation items are broken in Clearsilver templates |
|---|
Added a patch to fix this bug.
comment:4 by , 14 years ago
I tried to post this comment onto the SeaChange/WhatDevelopersWant page, but "Akismat" thinks it is spam.
Comment: Or look at #7418. A regression was introduced as part of the change in template systems, a patch was suggested as part of the bug report. While very "minor", it affected anyone using Clearsilver in plugins and could easily be applied and tested. That was almost 2 years ago! The only comment from a core developer is "provide a patch", which was almost immediately provided, even though the bug report had an inline diff provided for the fix. And now it is still waiting to be applied and released…
comment:5 by , 14 years ago
| Milestone: | next-minor-0.12.x |
|---|---|
| Resolution: | → wontfix |
| Status: | new → closed |
We almost never apply patches without testing them ourselves first.
At the time the patch was submitted, I no longer had a working setup with ClearSilver, nobody else from the team cared about it either. In retrospect, applying the patch anyway would probably have been very low risk, and worth doing it anyway. Maybe other factors were in play (a release was getting near, other more important fixes on the way, I can't remember).
Now it's certainly no longer worth applying it, as the next "change" we'll do in this area will be to remove the ClearSilver support completely.
comment:6 by , 14 years ago
As I wanted to add a few things to our patch guidelines, I found out that it was already written there: TracDev/SubmittingPatches#Whatisagoodpatch.
In particular, a patch is more like a prerequisite, not a sufficient condition.
comment:7 by , 13 years ago
I've just reproduced this on an instance of trac 0.12.2rc1, and solved it by applying the patch. So, I'd say this patch is still worth applying.
comment:8 by , 13 years ago
| Milestone: | → 0.12.3 |
|---|---|
| Resolution: | wontfix |
| Status: | closed → reopened |
Thanks for testing!
Note that ClearSilver support has indeed be removed now in newer versions of Trac, but as 0.12.x will therefore be the last version to support ClearSilver and that we got both a patch and testing feedback, let's finally apply this.
comment:11 by , 13 years ago
| Owner: | set to |
|---|
As we have an anonymous patch contributor here, I'll take the "blame" for the ticket…
Thanks for reminding us how ugly the ClearSilver templates were, sort of mitigates all the trouble we had with Genshi :P)
If anyone can make a clean patch (see TracDev/SubmittingPatches) and test the patch, we could eventually apply it.