On Wed, 2009-08-19 at 08:13 +0000, Jeroen T. Vermeulen wrote:
> Review: Needs Fixing
> Hi Salgado,
> 
> I stubbed my toe a few times looking at the UI changes, until I realized
> that I'm really missing a breadcrumb for the poll.  Once you've drilled
> down into a poll, everything you do in there refers back to the team.
> On the edit-option page I found no reference to the poll whatsoever:
> it's like you're editing a direct property of the team itself.  Having
> the poll shown as a first-class entity would make a big difference, I
> think.  But that's not for a template conversion to fix.

I think everybody who's used polls had a similar experience to yours.
They were written a long time ago and were left mostly untouched since
then, even when we redesigned navigation/UI for 1.0 and 2.0

> 
> There were a few small inconsistencies, or at least things that I think
> are inconsistent: adding an option gives me "Show options" and "Change
> details" as related pages, plus a Save button, but editing one gives me
> Save plus the Cancel link.  Is there a reason why these are so
> different?

That's because URL of these related pages are relative, so they work on
IPoll pages (like +newoption) but not on IPollOption ones (like
+option/id).  The Cancel link was added there only to make it possible
to return to the +options page, but all forms should have a Cancel link.

> 
> Something that may force the issue is that the "Change details" link on
> the new-option page is for the poll.  Had I not known where this UI
> element came from, I might have expected it to apply to the option I'm
> adding.  So I think it'd be more intuitive to remove these "related
> pages" links from the new-option page and add a Cancel link back to the
> poll instead.

I think that's a good thing to do.

> 
> Similarly, unless Martin has a rule against it, I think the poll edit
> form should have a Cancel link.  Right now there's no direct way back to
> the poll page.

They all should have a Cancel button.

> 
> (The same goes for the options overview by the way, which is the only
> place where I would really expect to be able to add or edit options.  It
> looks to me as though the options page wants to be integrated into the
> poll page, with an "Add new option" link inline and without any remnants
> of this old actions menu anywhere.  Maybe that's what you have planned
> anyway.)

In fact, my only plan is to migrate these pages without making things
any worse than they already are and spend the least possible amount of
time doing so.

But I really liked your suggestion, so I filed
https://bugs.edge.launchpad.net/bugs/415896 

> 
> Another very minor annoyance is that some of the page headings look a
> little redundant.  Sometimes they include the poll name, sometimes they
> don't.  Where they do, they duplicate the poll title displayed in small
> font right above the heading.  But I don't see any way of fixing that
> without either removing that small title (which seems to be baked into
> the application menu above it) or removing the poll name from the page
> title--both of which sound like bad alternatives.
> 
> Speaking of headings: your branch sets the label for the poll edit form
> to "Edit poll details."  What exactly that text should read is an
> ongoing discussion (started by Curtis on the mailing list), but more to
> the point, I don't see it show up anywhere!  If you provide a page_title
> in this view it'll get picked up (and the one in pagetitles.py will no
> longer be needed), but label doesn't seem to be.

The .label should be picked up by launchpad-form.pt, which is used in
these pages.  I can't seem to find what template picks up the page title
and includes it as an <h1>, though.  This must be something specific to
the locationless macro as I don't see this behaviour on other
LaunchpadFormViews.

Maybe Curtis can enlighten us.


> 
> Apart from that, the code as such looks good to me.  Nice use of a mixin
> for the navigation menu, though in this case I'd leave it out altogether
> instead of migrating it to the 3.0 setup.
> 

Agreed, but I don't really have the time to combine the pages and inline
the links in them, so the nav menus will have to do for now. :/

Thanks a lot for the review and the suggestions.


-- 
Guilherme Salgado <email address hidden>