Code review comment for lp:~bac/launchpad/lp3-announcements

Hi Brad.

This looks good. I have a few remarks. Since we are adding the index
page to this I am marking this

   review needs_fixing

On Wed, 2009-08-19 at 00:45 +0000, Brad Crittenden wrote:
> Thanks for the review Curtis.
...
> class AnnouncementCreateNavigationMenu(NavigationMenu,
> >> AnnouncementMenuMixin):
> >> + """A sub-menu for different aspects of modifying an
> >> announcement."""
> >> +
> >> + usedfor = IAnnouncementCreateMenu
> >> + facet = 'overview'
> >> + title = 'Create announcement'
> >> + links = ('announce', )xmlns:tal="http://xml.zope.org/namespaces/tal
> >> "
> >
> > Menus assume that links are a mutable list. This works, but may lead
> > to
> > cryptic errors if someone extends the menu,
>
> Really? From webapp/menu.py:
>
> assert isinstance(self.links, (tuple, list)), (
> "self.links must be a tuple or list.")
>
> Just grepping through registry/browser/*.py I see a few other examples
> of links being tuples. I've changed this one and the others in this
> module.

Ah! Thank you for showing me this. I am now understand the problem was
not the use if a tuple, but that the engineer assumed the menu he was
extending was a mutable list. You can use a list or a tuple as you want.

...

> > + <a tal:condition="context/url"
> > + tal:attributes="href context/url">
> > + <img src="/@@/link" alt="Link" /> Read more</a>
> >
> > This does not look right. We use sprites. The icon used is for
> > offsite links
> > which these announcements are not. Maybe we need an new sprite for
> > announcement. I think we should report a bug and assign it to Martin.
>
> It is an off-site link. There is no sprite for an off-site link.
> Should we go that route or just leave this as is?

Yes, there is a sprite. The external links on the project page use it.
'external-link' which is the globe the link us showing now.

...
...

> class AnnouncementContextMenu(ContextMenu, AnnouncementMenuMixin):
> """The menu for working with an Announcement."""
> usedfor = IAnnouncement
> - links = ('edit', 'retarget', 'publish', 'retract', 'delete')
> + links = ['edit', 'retarget', 'publish', 'retract', 'delete']

I think we can delete this class. I do not think it will be used since
we have converted all the pages that use this with the index page

...

> === modified file 'lib/lp/registry/templates/hasannouncements-index.pt'
> --- lib/lp/registry/templates/hasannouncements-index.pt 2009-08-17 20:36:01 +0000
> +++ lib/lp/registry/templates/hasannouncements-index.pt 2009-08-19 00:43:48 +0000
> @@ -17,6 +17,7 @@
> <h1>News and announcements</h1>
> </tal:heading>
> <div metal:fill-slot="main">
> + <br />

This looks wrong. <br> are not meant to create blank lines. They so not
belong between block elements because block elements define margins and
padding.

I think you want the op content, or all the content wrapped in a
<div class="top-portlet"> which sets margins and padding.

--

__C U R T I S C. H O V E Y_______
Guilty of stealing everything I am.