Code review comment for lp://staging/~allenap/launchpad/ui-convert-bug-tracker-3.0-bug-418155-pt2

> On Tue, 01 Sep 2009 11:43:19 -0000
> Michael Nelson <email address hidden> wrote:
>
> > Review: Needs Fixing ui
> > > To walk through the UI, try:
> >
> > Thanks for providing these walk-throughs... much easier to review!
> > I'll start doing that too. And sorry for the two 'needs fixing'
> > reviews! The pages really do look great (we initially tried to
> > re-use our soyuz context portlets in the same way), but afaik, it's
> > left-over from 1.0 (or maybe I'm confused at that point - I wasn't
> > around then), but it's not meant to be in the 3.0 side-bar.
>
> No worries about the needs-fixing. They need fixing! :)
>
> >
> > >
> > > * Go to the bug trackers page (for IBugTrackerSet) at
> > > <https://bugs.launchpad.dev/bugs/bugtrackers>,
> > >
> > > * There's nothing new in this page for this branch, so just click on
> > > "This Mozilla.prg Bug Tracker" to see its overview page,
> > >
> > > * Look at the portlets. The top portlet replicates some of the info
> > > that's in the page, but it is used elsewhere in Launchpad so it's
> > > useful to keep.
> >
> > See comments on the first MP. I think it's really 1.0 templates
> > trying to be forced into the 3.0 mold (I tried the same thing with
> > some soyuz pages and got rejected ;) ).
>
> Yes, I've removed all the side portlets from all of the bug tracker
> pages; they don't make sense anywhere. The navigation menu for the bug
> tracker index page remains.

Looks good. I just noticed that you removed the colons from the dt's in the details portlet. I have no idea what is correct, but I usually just do what registry has done (ie. https://launchpad.dev/firefox ). Up to you.

>
> >
> > >
> > > * Click "Change details",
> > >
> > > * This is one place where the details portlet is useful.
> >
> > Well, not really - the only extra information presented in the
> > portlet is not really relevant to the person editing the watch?
> > Otherwise it's just duplicated info, and as mentioned in the
> > previous MP, afaik, isn't the type of content that is meant to go in
> > the side portlet. I'd recommend using main_only for this template,
> > you might even be able to get rid of the template and use
> > generic-edit.pt?
>
> I'd not heard of generic-edit before; it'll be handy for the
> future. However, I don't think it'll be suitable here because the edit
> page has some blurb at the beginning, including an explanation of why
> a particular bug tracker cannot be deleted (which is not static).

Sure - I don't mind either way, but just note that there is an extra_info slot on launchpad-form.pt (that is used by generic-edit.pt).

>
> >
> > >
> > > * Use the new cancel link to return to the previous page,
> >
> > Great!
> >
> > >
> > > * Notice the new h1 title and h2 "Summary" heading,
> >
> > Great! Note that both of these above two points will happen
> > automatically with generic-edit.
> >
> > >
> > > * Append /42 to the URL and see the remote bug index page, including
> > > the h2 "Is watched by..." and the new text. I think the text is
> > > good, but the heading was a tough one. I'm simply still not sure
> > > exactly how to use the heading slot.
> >
> >
> > Yeah, it's been confusing and could change again - but afaik, you've
> > done the right thing for the current rules (ie. this is an +index
> > page, so putting an <h1>context.title</h1> is correct).
> >
> > I'm not so sure about "Is watched by...". I think this h2 shouldn't
> > be a continuation of a sentance. In fact, I'm not even sure that the
> > h2 is necessary. I know I'm looking at the Remote bug #42 for the
> > Mozilla.org bug tracker, and your overview para tells me exactly
> > what I need to know.
>
> I've removed the h2. You're right, the page does not need it at
> all. I've also changed the table - which contained one sparsely filled
> column - to a ul, and have switched the custom formatted bug links to
> use fmt:link (except in the case of private bugs, which do still have
> custom formatting, but it's pretty simple and does now use sprites).

Nice.

>
> >
> > And again, I don't think we can put the context details portlet in
> > the side-bar (it was requested for 1.0 and afaik meant to be removed
> > for 2.0(?) and doesn't belong in the side-bar for 3.0).
>
> I've killed it!
>
> I've attached post-ui-review.diff, which covers the changes made
> following your UI review of both this branch and the parent branch.

Thanks Gavin.