Code review comment for lp:~michael.nelson/launchpad/sprint-index-and-attend-3.0

Barry Warsaw wrote:
> Review: Approve code ui*
> Hi Michael,
>
> Thanks for working on this branch. I'm psyched to see us nailing down these
> blueprint pages as we finish up 3.0.
>
> As mentioned in irc, sprint-index doesn't look quite right because of the
> breadcrumbs that shouldn't be there. I have a hack, er, solution for that
> attached below. It basically puts a null +hierarchy view on the page. We
> might want to consider generalizing that or providing an easier way to do it.
> If you want to open a bug asking for that, I might be able to fix it
> post-3.0. For now, this hack will work.

Thanks for the work-around!

>
> First, let's look at the code. I'll omit the stuff that looks fine.
>
> === modified file 'lib/lp/blueprints/browser/sprint.py'
> --- lib/lp/blueprints/browser/sprint.py 2009-09-16 18:18:49 +0000
> +++ lib/lp/blueprints/browser/sprint.py 2009-09-18 10:49:12 +0000
>> @@ -69,7 +70,7 @@
>> usedfor = ISprint
>>
>>
>> -class SprintOverviewMenu(ApplicationMenu):
>> +class SprintOverviewMenu(NavigationMenu):
>
> While you're here can you give this class a docstring?

Done.

>
> === modified file 'lib/lp/blueprints/stories/sprints/05-sprint-creation.txt'
> --- lib/lp/blueprints/stories/sprints/05-sprint-creation.txt 2009-09-15 10:26:04 +0000
> +++ lib/lp/blueprints/stories/sprints/05-sprint-creation.txt 2009-09-18 11:31:27 +0000
>> @@ -80,14 +80,9 @@
>> Since the sprint's time zone was set to UTC, the dates are displayed in
>> that time zone:
>>
>> - >>> print find_portlet(user_browser.contents, 'Meeting details')
>> - <...
>> - ...Starts:...
>> - 2006-10-10 09:15 UTC...
>> - ...Ends:...
>> - 2006-10-13 16:00 UTC...
>> - ...
>> -
>> + >>> print extract_text(find_tag_by_id(user_browser.contents, 'start-end'))
>> + Starts: 09:15 UTC on Tuesday, 2006-10-10
>> + Ends: 16:00 UTC on Friday, 2006-10-13
>
> Thanks for cleaning these up!
>
> === modified file 'lib/lp/blueprints/stories/sprints/10-sprint-editing.txt'
> --- lib/lp/blueprints/stories/sprints/10-sprint-editing.txt 2009-09-16 17:32:51 +0000
> +++ lib/lp/blueprints/stories/sprints/10-sprint-editing.txt 2009-09-18 10:49:12 +0000
>> @@ -2,8 +2,8 @@
>> available to those who have permissions to edit that sprint.
>>
>> >>> anon_browser.open('http://launchpad.dev/sprints/ubz')
>> - >>> anon_browser.title
>> - 'Ubuntu Below Zero (sprint or meeting)'
>> + >>> print anon_browser.title
>> + Ubuntu Below Zero : Meetings
>
> And these. If you have the inclination, can you indent the interactive
> prompts 4 spaces?

Done.

>
> === modified file 'lib/lp/blueprints/stories/sprints/sprint-settopics.txt'
> --- lib/lp/blueprints/stories/sprints/sprint-settopics.txt 2009-08-13 19:03:36 +0000
> +++ lib/lp/blueprints/stories/sprints/sprint-settopics.txt 2009-09-18 10:49:12 +0000
>> @@ -55,8 +55,8 @@
>> 'http://launchpad.dev/sprints/uds-guacamole'
>>
>> >>> ut = 'Ubuntu Team'
>> - >>> meeting_portlet = find_portlet(browser.contents, 'Meeting details')
>> - >>> ut in extract_text(meeting_portlet.find(text='Driver:').findNext('a'))
>> + >>> meeting_drivers = find_tag_by_id(browser.contents, 'meeting-drivers')
>> + >>> ut in extract_text(meeting_drivers.findNext('a'))
>> True
>
> Is it possible to just print the text and use ellipses to omit anything you
> don't care about? That would be better for documentation/testing purposes
> than just printing True.

Done.

>
> Here's a diff that will fix the breadcrumbs. It might need a little cleaning
> up, but should give you the idea.

Thanks! The only strange thing with your diff is that adding the
view.label ensures that a second H1 appears below the tabs, whereas I
understood the heading rules to mean that, since the h1 is above the
tabs (as this is the index of a root context), there should be no
further structural headings (or breadcrumbs - thanks). So I removed the
label.

Also, adding the display_breadcrumbs = False affects other sub pages - I
simply had to add a page_title for the +edit page. And the +attend page
lost it's reverse-breadcrumb title too. Is there a way around this once
you set a breadcrumb in the hierarchy to not display?

Back on the index page, I also turned the meeting drivers into a
separate portlet to divide up the top-portlet which makes it easier to
scan the page.

>
> With consideration of these comments, merge-conditional, r=me.
>
>
> === modified file 'lib/lp/blueprints/browser/configure.zcml'
> --- lib/lp/blueprints/browser/configure.zcml 2009-09-17 21:20:14 +0000
> +++ lib/lp/blueprints/browser/configure.zcml 2009-09-18 15:54:43 +0000
> @@ -23,6 +23,13 @@
> for="lp.blueprints.interfaces.sprint.ISprint"
> path_expression="name"
> parent_utility="lp.blueprints.interfaces.sprint.ISprintSet"/>
> + <browser:page
> + for="lp.blueprints.interfaces.sprint.ISprint"
> + name="+hierarchy"
> + class="lp.blueprints.browser.sprint.SprintIndexHierarchy"
> + template="../../app/templates/launchpad-hierarchy.pt"
> + permission="zope.Public"
> + />
> <browser:pages
> for="lp.blueprints.interfaces.sprint.ISprint"
> class="lp.blueprints.browser.sprint.SprintView"
>
> === modified file 'lib/lp/blueprints/browser/sprint.py'
> --- lib/lp/blueprints/browser/sprint.py 2009-09-18 10:49:12 +0000
> +++ lib/lp/blueprints/browser/sprint.py 2009-09-18 15:56:04 +0000
> @@ -10,6 +10,7 @@
> 'SprintBrandingView',
> 'SprintEditView',
> 'SprintFacets',
> + 'SprintIndexHierarchy',
> 'SprintMeetingExportView',
> 'SprintNavigation',
> 'SprintOverviewMenu',
> @@ -47,6 +48,7 @@
> LaunchpadFormView, LaunchpadView, Link, Navigation, NavigationMenu,
> StandardLaunchpadFacets, action, canonical_url, custom_widget,
> enabled_with_permission)
> +from canonical.launchpad.browser.launchpad import Hierarchy
> from canonical.launchpad.webapp.batching import BatchNavigator
> from canonical.launchpad.webapp.breadcrumb import Breadcrumb
> from canonical.launchpad.helpers import shortlist
> @@ -151,12 +153,20 @@
> enable_only = ['overview', ]
>
>
> +class SprintIndexHierarchy(Hierarchy):
> + @property
> + def display_breadcrumbs(self):
> + return False

I just used display_breadcrumbs = False here.

> +
> +
> class SprintView(HasSpecificationsView, LaunchpadView):
>
> __used_for__ = ISprint
>
> implements(IMajorHeadingView)
>
> + label = 'Meeting overview'
> +

And removed this label so as to not get two h1's. 'Meeting overview' was
not a structural heading anyway, it was just the normal h2 heading of
the first portlet, but it doesn't seem necessary anyway (an we don't
seem to use headings in the top-portlet elsewhere).

> def initialize(self):
> self.notices = []
> self.latest_specs_limit = 5
>
> === modified file 'lib/lp/blueprints/templates/sprint-index.pt'
> --- lib/lp/blueprints/templates/sprint-index.pt 2009-09-18 10:49:12 +0000
> +++ lib/lp/blueprints/templates/sprint-index.pt 2009-09-18 15:58:21 +0000
> @@ -22,8 +22,6 @@
> has_any_specs view/has_any_specifications">
>
> <div class="top-portlet">
> - <h2>Meeting overview</h2>
> -
> <p>
> <span tal:replace="context/summary">
> Sprint Summary Goes Here
>
>

Thanks Barry - incremental attached.

I'll send it off to ec2test now, but not to land, just in case you're
around and have any changes. I'll land it if the tests all succeed.

--
Michael