Edgewall Software
Modify

Opened 10 years ago

Last modified 4 months ago

#8989 new enhancement

Milestones should have a 'Back to Roadmap' link in the contextual navigation

Reported by: Ryan J Ollos Owned by:
Priority: low Milestone: next-dev-1.5.x
Component: roadmap Version: 0.12dev
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:

Description

When navigating to a milestone from the roadmap, there should be a Back to Roadmap link displayed in the contextual navigation menu.

This is not currently implemented in Trac 0.12dev, as can be seen: milestone:0.12-multirepos

This seems simple enough that I'll try to implemented it and provide a patch.

Attachments (6)

RoadmapWithAnchors.png (7.9 KB ) - added by Ryan J Ollos <ryano@…> 10 years ago.
roadmapnav.patch (6.4 KB ) - added by Ryan J Ollos <ryano@…> 10 years ago.
t8989-r11483-1.patch (3.5 KB ) - added by Ryan J Ollos <ryan.j.ollos@…> 7 years ago.
Patch against r11483 of the trunk.
t11014-r11483-2.patch (1.2 KB ) - added by Ryan J Ollos <ryan.j.ollos@…> 7 years ago.
Patch against r11483 of the trunk.
t8989-r11483-2.patch (5.6 KB ) - added by Ryan J Ollos <ryan.j.ollos@…> 7 years ago.
Patch against r11483 of the trunk.
t8989-r11483-3.patch (5.5 KB ) - added by Ryan J Ollos <ryan.j.ollos@…> 7 years ago.
Patch against r11483 of the trunk.

Download all attachments as: .zip

Change History (28)

comment:1 by Remy Blank, 10 years ago

Milestone: next-minor-0.12.x

I guess the reason why there is no such link is that you can just click on "Roadmap". However, I always have to think twice before doing that, because I'm used to the "Back to …" links in the ctxtnav. So I'm +1 for adding that link.

Bonus points if you implement it so that:

  • The link leads back to the same milestone in the roadmap. You will need to add anchors in the roadmap, as there are currently none.
  • The "Show already completed milestones" checkbox is selected if the milestone is closed.

Scheduling for next-minor-0.12.x until a patch is available.

comment:2 by Christian Boos, 10 years ago

We had a "mid-air collision" :-)


There's already the main navigation item Roadmap for this, which moreover is highlighted when you're looking at Milestone, so I don't see what a Back to Roadmap contextual link would add.

Generally speaking, the contextual "Back" is used when looking at a resource from a particular selection (e.g. a ticket from a query or report), or a child resource (e.g. back to the parent resource from an attachment).


But yes, going back to the actual position of the milestone within the roadmap and activating Show closed milestones when seeing that "#themilestone" is actually closed could be an added value justifying the feature…

comment:3 by anonymous, 10 years ago

Good points … I see now why this wasn't implemented already. I have the th:MenusPlugin installed so the Roadmap link is buried in a submenu, and I usually navigate via the contextual navigation menu, which is why I was expecting a link to be there.

I will work on implementing it with the proposed features (navigate back to specific milestone incl a closed milestone), but probably won't get to this until next weekend.

We have a lot of milestones in my Trac instance, so I really like the idea of navigating back to a specific milestone in the roadmap.

comment:4 by ryano@…, 10 years ago

Whoops … Should have logged in before posting that comment, but it was probably obvious that the comment was from the reporter.

by Ryan J Ollos <ryano@…>, 10 years ago

Attachment: RoadmapWithAnchors.png added

by Ryan J Ollos <ryano@…>, 10 years ago

Attachment: roadmapnav.patch added

comment:5 by Ryan J Ollos <ryano@…>, 10 years ago

I have a working version of a patch that still has at least some minor issues to be worked out, and I'd really appreciate some feedback on it.

I'm storing the roadmap filter state in the req.session, so that it behaves more like the timeline filter with regard to remembering a user's selection. Now, one can navigate away from the roadmap page, and when returning to the roadmap page the selection for Show completed milestones will still have the same value. Previously it would always be unselected. After the patch in #9036 is applied, I'll need to modify this patch to support the second filter.

There are currently two issues:

  1. If I navigate to a roadmap entry, e.g. Develop project plan, the request is for /milestone/Develop project plan. I select Back to Roadmap and the request is for /roadmap#Develop project plan. So far, things seem fine. Now I scroll up to the preferences box, check Show completed milestones and press update; the request is for /roadmap?showcompleted=on&update=Update#Develop project plan. It seems to work, but I was expecting something more like /roadmap#Develop project plan?showcompleted=on&update=Update.
  2. The anchor appears on a separate line from the milestone heading as shown in the image below.

This is the biggest task I have tried to do in python so far, so I'm prepared for the fact that this might need significantly more work.

in reply to:  5 comment:6 by Christian Boos, 10 years ago

Replying to Ryan J Ollos <ryano@…>:

… the request is for /roadmap?showcompleted=on&update=Update#Develop project plan. It seems to work, but I was expecting something more like /roadmap#Develop project plan?showcompleted=on&update=Update.

This is how it's supposed to work (anchor has to come after the request parameters, see rfc:2396#section-4).

  1. The anchor appears on a separate line from the milestone heading as shown in the image below.

This is indeed problematic. Maybe look if there's any structural differences in the resulting HTML between this and e.g. the Wiki section links. If not, then try to identify the CSS differences (the Web Inspector in Chrome or Firebug in Firefox are invaluable tools for troubleshooting this kind of annoyance).

Otherwise the patch looks quite good. I'll add a few comments about the parts that can nevertheless be improved later today. Keep up the good work!

comment:7 by Christian Boos, 9 years ago

Milestone: next-minor-0.12.xnext-major-0.1X

Btw, the highlighting idea of ticket:7573#comment:12 would apply here as well.

comment:8 by anonymous, 7 years ago

The Back to Roadmap contextual navigation link was added in #9820 / [10321]. There are a couple of additional features that could be addressed in this ticket, as noted in comment:1.

comment:9 by anonymous, 7 years ago

The patch has been refreshed and now provides the following features:

  • The Back to roadmap link on the milestone page leads back to the same milestone in the roadmap.
  • Milestones on the roadmap have anchors.
  • The milestone is highlighted on the roadmap (comment:7) when navigating to a specific milestone.
  • The Show completed milestones milestones is selected if the milestone is closed. Unlike the earlier path, the roadmap filter states are not stored in the session data.

The patch also fixes some coding style issues for readable strings (replacing single quotes with double quotes). The functional tests have been executed and pass, although this patch does not add any functional tests. I'd be happy to add some if the patch is deemed acceptable and will be committed.

by Ryan J Ollos <ryan.j.ollos@…>, 7 years ago

Attachment: t8989-r11483-1.patch added

Patch against r11483 of the trunk.

comment:10 by Christian Boos, 7 years ago

Milestone: next-major-releasesnext-dev-1.1.x
Owner: set to Christian Boos
Status: newassigned

Looks good! Thanks for the patch, I'll test later today.

comment:11 by Remy Blank, 7 years ago

I believe the anchor of the href for the up link is missing some form of URL-encoding, as it's added outside of the ref.href.roadmap() call.

comment:12 by Ryan J Ollos <ryan.j.ollos@…>, 7 years ago

I'll have to look more closely, but it looks like maybe I should be calling trac.util.text.unicode_quote. What do you thinking about adding support to the Href class for either handling a kw argument named fragment, or special handling of string arguments that begin with #?

comment:13 by Christian Boos, 7 years ago

The fragment keyword argument is an interesting idea, though we currently don't do much normalization of the fragment. And this doesn't seem to be a problem in practice, as even the validators don't seem to bother much about the precise rules for href fragments.

For example, even an URL ending with /roadmap#123#繁體中文 works fine for IE[7:10], FF, Opera and Chrome, the W3C XHTML validator doesn't complain, neither does LXML. So I don't think we should be more strict than these tools are.

OTOH, we may want to be stricter for what we put in the id attribute, as there we have to be more careful if we want to remain XHTML 1.0 compliant. But maybe our efforts should be better spent in migrating towards HTML5 anyway… which I suppose is already the approach you opted for, with replace(' ', ''). That might go in a helper function regardless what we decide (trac.util.html.normalize_id?).

comment:14 by Ryan J Ollos <ryan.j.ollos@…>, 7 years ago

It sounds like I should do the following then:

  • Add a new function trac.util.html.normalize_id which for now only calls replace(' ', '').
  • Call trac.util.html.normalize_id in the two places that I currently call replace(' ', '').

Should I pass trac.util.html.normalize_id in the data for this template, or add it to the default context data?

in reply to:  14 comment:15 by Christian Boos, 7 years ago

Replying to Ryan J Ollos <ryan.j.ollos@…>:

It sounds like I should do the following then:

Fine by me.

Should I pass trac.util.html.normalize_id in the data for this template, or …

Or pass the html module itself, perhaps as util_html (or instead of .html, put it in .presentation and pass .presentation itself?).

by Ryan J Ollos <ryan.j.ollos@…>, 7 years ago

Attachment: t11014-r11483-2.patch added

Patch against r11483 of the trunk.

by Ryan J Ollos <ryan.j.ollos@…>, 7 years ago

Attachment: t8989-r11483-2.patch added

Patch against r11483 of the trunk.

by Ryan J Ollos <ryan.j.ollos@…>, 7 years ago

Attachment: t8989-r11483-3.patch added

Patch against r11483 of the trunk.

comment:16 by Ryan J Ollos <ryan.j.ollos@…>, 7 years ago

t8989-r11483-2.patch has the changes discussed, but puts presentation in the default context data. If we go with this patch, then there are 7 functions from presentation that are passed individually. Would it be worthwhile to deprecate the use of those 7 functions and make changes through the codebase to use presentation.<function> rather than <function>?

t8989-r11483-3.patch has the changes discussed and puts presentation in the roadmap template data.

comment:17 by anonymous, 6 years ago

Hm, why add a second Roadmap link to the page, directly under the existing Roadmap link, with exactly the same functionality?

If it were a history.back(-1) or history.back(-2) then it would make sense as the user agent would bring me back to the same scroll offset in the road map's milestone list.

Right now, using the back button seems more useful to me.

in reply to:  17 comment:18 by anonymous, 6 years ago

Replying to anonymous:

Ups, just saw that it is not in the T.E.O. version but that work is on the way to make this different from the original Roadmap link.

comment:19 by Ryan J Ollos, 6 years ago

Reporter: changed from Ryan Ollos <ryano@…> to Ryan J Ollos

comment:20 by Ryan J Ollos, 5 years ago

Owner: Christian Boos removed
Status: assignednew

comment:21 by Ryan J Ollos, 4 years ago

Milestone: next-dev-1.1.xnext-dev-1.3.x

Narrowing focus for milestone:1.2. Please move ticket to milestone:1.2 if you intend to fix it.

comment:22 by Ryan J Ollos, 4 months ago

Milestone: next-dev-1.3.xnext-dev-1.5.x

Milestone renamed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned. Next status will be 'new'.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.