Hi Brad.

This is very nice. I think we should tweak the layout a bit before
landing this. I explain XMLLint's spazzing below.

> === modified file 'lib/canonical/launchpad/icing/style-3-0.css'
> --- lib/canonical/launchpad/icing/style-3-0.css	2009-08-17 11:22:39 +0000
> +++ lib/canonical/launchpad/icing/style-3-0.css	2009-08-17 20:36:01 +0000
...

> @@ -581,6 +581,10 @@
>      border-top: 1px solid #d0d0d0;
>      border-bottom: 1px solid #d0d0d0;
>      }
> +.announcement .registered {
> +    font-size: 95%;

This is not a support font-size you must use one of the percentages
from the table to guarantee that they display the same on all platforms.

    font-size: 93%;

is official.

> +    margin-top: -2px

Missing semi-colon.

+    }
 
...

> === modified file 'lib/lp/registry/browser/announcement.py'
> --- lib/lp/registry/browser/announcement.py	2009-08-11 04:00:47 +0000
> +++ lib/lp/registry/browser/announcement.py	2009-08-17 20:36:01 +0000

...

> @@ -70,6 +69,12 @@
>          text = 'Delete announcement'
>          return Link('+delete', text, icon='trash-icon')
>  
> +    @enabled_with_permission('launchpad.Edit')
> +    def announce(self):
> +        text = 'Make announcement'
> +        summary = 'Publish an item of news for this project'
> +        return Link('+announce', text, summary, icon='add')

We use publish to describe the act of making public, not creating.

...

> @@ -100,6 +105,19 @@
>              self.context = context
>  
>  
> +class IAnnouncementCreateMenu(Interface):
> +    """A marker interface for creation announcement navigation menu."""
> +
> +
> +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,

...

> === modified file 'lib/lp/registry/stories/announcements/xx-announcements.txt'
> --- lib/lp/registry/stories/announcements/xx-announcements.txt	2009-08-13 18:10:15 +0000
> +++ lib/lp/registry/stories/announcements/xx-announcements.txt	2009-08-17 20:36:01 +0000

...

> @@ -418,47 +419,16 @@
>  Editing announcements
>  ---------------------
...

> -If you have the necessary privileges you will see links to edit the
> -announcements.
> +To edit an announcement you must go to the individual announcement
> +page and then follow the proper links.

Me? I am not doing anything. I think No Privileges Person is doing this.
 
>      >>> priv_browser.open('http://launchpad.dev/tomcat/+announcements')
> -    >>> listed_announcements = find_tag_by_id(
> -    ...    priv_browser.contents, 'announcements')
> -    >>> edit_links = listed_announcements.fetch('a', text=edit_link_text)
> -    >>> len(edit_links)
> -    2
> -
> -Following a link takes you to a form where you can change the headline,
> -summary and URL of the announcement:
> -
> -    >>> 'Modified headline' not in announcements(priv_browser.contents)
> -    True
> -    >>> 'Modified summary' not in announcements(priv_browser.contents)
> -    True
>      >>> print priv_browser.getLink('Read more').url
>      http://apache.org/announcement/rocking/
> -    >>> priv_browser.getLink(edit_link_text).click()
> +    >>> priv_browser.getLink('Apache announcement headline').click()
> +    >>> priv_browser.getLink('Modify announcement').click()
>      >>> print priv_browser.url
> -    http://launchpad.dev/apache/+announcement/...
> +    http://launchpad.dev/apache/+announcement/.../+edit

I think the user cares about the title, not the url.

...

> === modified file 'lib/lp/registry/templates/announcement-listing-detailed.pt'
> --- lib/lp/registry/templates/announcement-listing-detailed.pt	2009-07-17 17:59:07 +0000
> +++ lib/lp/registry/templates/announcement-listing-detailed.pt	2009-08-17 20:36:01 +0000
> @@ -1,59 +1,47 @@

You are missing

    xmlns:tal="http://xml.zope.org/namespaces/tal"
    i18n:domain="launchpad"

in the root element, which xmllint considers to be heresy.

...

> +<div class="portlet" tal:attributes="id string:${context/id}">

I do not believe this is a valid id in XML/CSS. An id must start with
a letter you can use

    tal:attributes="id context/id/fmt:css-id"

to guarantee a valid id.

> +  <div class="announcement">
> +    <a tal:attributes="href string:${context/fmt:url}">
> +      <h2>
> +        <tal:date tal:condition="context/date_announced">
> +          <span tal:replace="context/date_announced/fmt:date"/>:
> +        </tal:date>
> +        <tal:title content="context/title">News item title</tal:title>
> +      </h2>
> +    </a>

Th anchor element cannot contain a block element. The anchor must go inside
the <h2>. I could not see this was a link. maybe it is not important since
we have read more below.

> +    <p class="registered">
> +      Written for
> +      <tal:pillar replace="structure context/target/fmt:link" />
> +      by
> +      <tal:registrant replace="structure context/registrant/fmt:link" />.
> +    </p>
> +
> +    <tal:summary condition="context/summary"
> +      tal:replace="structure context/summary/fmt:text-to-html" />

Is this guaranteed to make a block-element? I do not think it does, which
will lead to unpredictable spacing. You should wrap this in a <div>.

/me looks.

No. place this in a div to ensure inline content does not mix with
block content.

> +    <p>
+      <tal:future condition="context/future">
+        <img src="/@@/warning" title="Announcement is not yet public." />
+        <strong tal:condition="context/active">
+          <tal:date_set condition="context/date_announced">
+            This announcement will be published on
+            <span tal:replace="context/date_announced/fmt:datetime" />.
+          </tal:date_set>
+        </strong>
+      </tal:future>
+      <strong tal:condition="not: context/active">
+        <img src="/@@/info" title="Announcement has been retracted." />
+          Retracted.
+      </strong>
+      <strong tal:condition="not: context/date_announced">
+          No publishing date set.
+      </strong>
+      <strong tal:condition="context/date_last_modified">
+          Updated
+          <span tal:replace="context/date_last_modified/fmt:displaydate" />.
+      </strong>
+      <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.

...

> === modified file 'lib/lp/registry/templates/hasannouncements-index.pt'
> --- lib/lp/registry/templates/hasannouncements-index.pt	2009-07-17 17:59:07 +0000
> +++ lib/lp/registry/templates/hasannouncements-index.pt	2009-08-17 20:36:01 +0000
> @@ -3,53 +3,73 @@
>    xmlns:tal="http://xml.zope.org/namespaces/tal"
>    xmlns:metal="http://xml.zope.org/namespaces/metal"
>    xmlns:i18n="http://xml.zope.org/namespaces/i18n"

...

> +  metal:use-macro="view/macro:page/main_side"
> +  i18n:domain="launchpad">
> +
> +  <metal:atom fill-slot="head_epilogue">
> +    <link rel="alternate" type="application/atom+xml" tal:attributes="
> +               href view/feed_url;
> +               title string:Announcements for ${context/displayname}" />
> +  </metal:atom>
> +
> +  <body>
> +    <tal:heading metal:fill-slot="heading">
> +      <h1>News and announcements</h1>
> +    </tal:heading>
> +
> +    <div metal:fill-slot="main">
> +      <p tal:condition="not: view/announcement_nav/batch/listlength">
> +        There are no announcements for this project.
> +      </p>

This page looks like it is missing something. The announcement top-border
is under the <h1>, separating the content from the heading. We can fix this
either by adding an explanatory paragraph about this page or we add
a rule...

> +      <div tal:condition="view/announcement_nav/batch/listlength"
> +           id="announcements">
> +        <tal:per_announcement
> +           repeat="announcement view/announcement_nav/currentBatch"
> +           replace="structure announcement/@@+listing-detailed" />

... here to set set the class of the first announcement to top-portlet.

> +      </div>
> +      <div class="lesser"
> +           tal:content="structure view/announcement_nav/@@+navigation-links-lower"
> +           />
> +    </div>
> +
> +    <tal:side metal:fill-slot="side"
> +              define="announcements view/announcement_nav/currentBatch;
> +                      overview_menu context/menu:overview">

I do not think you use an overview_menu.

> +      <tal:menu replace="structure view/@@+global-actions" />
> +
> +      <div class="portlet">
> +        <h2>
> +          <span style="float: right;">

Do we have a class="right"? No I guess not. This is fine I think

> +            <a title="Atom 1.0 feed"
> +               tal:attributes="href view/feed_url"><img src="/@@/rss.png"/></a>
> +          </span>
> +          Announcements
> +        </h2>
> +
> +        <p tal:condition="not: view/announcement_nav/batch/listlength">
> +          <tal:name replace="context/displayname" /> has no announcements.
> +        </p>

I think this is stated in the main content. We should not need this.

> +        <tal:announcements tal:condition="view/announcement_nav/batch/listlength">

This condition can be moved into the <ul>
> +          <ul>
> +            <li tal:repeat="announcement announcements">
> +              <img src="/@@/warning" alt="[Not published]"
> +                   title="This is not yet a public announcement"
> +                   tal:condition="not: announcement/published" />
> +              <a tal:attributes="href string:#${announcement/id}"

    <a tal:attributes="href announcement/id/fmt:css-id"


> +                 tal:content="announcement/title">
> +                Announcement title
> +              </a>
> +              <strong
> +                 tal:condition="announcement/date_announced"
> +                 tal:content="announcement/date_announced/fmt:displaydate" />
> +              <br />
> +              <br />

Two <br /> in a row is a classic symptom implementation flaw. If you want
a block element use one. In this case I don't think either is needed. We
can look at adjusting the padding or margins of the <li>

/me looks

Yes too much space no more than 0.5em; should separate list items. A full
em (or more than that in this case) breaks the list. Try:

    margin-bottom: .5em;

on the <li>. It is better to have than in the stylesheet if we can isolate
the rule

> +            </li>
> +          </ul>
> +        </tal:announcements>
> +      </div>
> +    </tal:side>
> +  </body>
 </html>