Thanks for the review Curtis.

On Aug 18, 2009, at 11:37 , Curtis Hovey wrote:

> Review: Needs Fixing code
> Hi Brad.
>
> This is very nice. I think we should tweak the layout a bit before
> landing this. I explain XMLLint's spazzing below.

Great.

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

Didn't see that comment in the CSS.  Thanks for catching it.

>
>> +    margin-top: -2px
>
> Missing semi-colon.
>

Duh.

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

'Create an item...'

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

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.

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

reworded.

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

I was following the existing style of the test.

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

I was confused about this so I tried both ways.  The presentation here  
was better visually, I thought, but I didn't know it was invalid.   
I'll fix it.

"Read more" does not refer to the item, it is the URL of an *external*.

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

Done

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

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?

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

I did that and it had no visible affect.  Hmm.

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

gone

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

It was repeated here so this portlet would more closely match the  
announcements portlet on the product index page.  If we get rid of the  
message should we also get rid of the entire portlet, leaving only the  
'Make announcement' portlet in the side?

I got rid of it as we discussed on IRC

>
>> +        <tal:announcements tal:condition="view/announcement_nav/ 
>> batch/listlength">
>
> This condition can be moved into the <ul>

Actually it went up higher.

>> +          <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;

This seems to work after removing both <br />

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

I'll post an incremental diff next.

--Brad