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

On Aug 19, 2009, at 16:30 , Curtis Hovey wrote:

> Review: Needs Fixing
> 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

Sure, that work is included in the diff I've got now.

>
> On Wed, 2009-08-19 at 00:45 +0000, Brad Crittenden wrote:
>
>>> + <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.

Thanks for the pointer. I see it uses 'no-follow' too, which I
definitely should have here.

>
> ...
> ...
>
>> 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

Good call

>
> ...
>
>> === 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.

I inserted <div class="top-portlet" style="margin-top: 0.5em;"> which
seems to give a reasonable amount of spacing.

In the update I've also updated announcement-index.pt and made a fix
where the index page for a distribution was getting messed up due to
changes to HasAnnouncementsView (it is marked with an XXX that needs
to be removed when the update to distribution happens).