Launchpad itself

Code review comment for lp://staging/~cprov/launchpad/bug-412715-registration-info-in-base-layout

  1. bug-412715-registration-info-in-base-layout
  2. Merge into devel
Revision history for this message
Celso Providelo (cprov) wrote :

On Wed, Sep 16, 2009 at 6:38 PM, Edwin Grubbs
<email address hidden> wrote:
> Review: Approve
> Hi Celso,

Hi Edwin,

> This branch looks like a nice improvement. Just a couple of comments below.
>
> merge-conditional

Thank you.

> -Edwin
>
>
>>=== modified file 'lib/lp/app/browser/tests/base-layout.txt'
>>--- lib/lp/app/browser/tests/base-layout.txt   2009-09-12 01:39:04 +0000
>>+++ lib/lp/app/browser/tests/base-layout.txt   2009-09-16 20:19:40 +0000
>>@@ -93,7 +93,10 @@
>>     ...
>>               <div class="yui-b" dir="ltr">
>>                 <div>
>>-                <h2>Heading</h2>
>>+                  <div class="registering">
>>+                    <div />
>>+                  </div>
>>+                  <h2>Heading</h2>
>>                   <ol class="breadcrumbs" xmlns="http://www.w3.org/1999/xhtml">
>>     ...
>>
>>@@ -189,6 +192,9 @@
>>     <BLANKLINE>
>>               <div class="yui-b" dir="ltr">
>>                 <div>
>>+                  <div class="registering">
>>+                    <div />
>>+                  </div>
>>     <BLANKLINE>
>>                   <ol class="breadcrumbs" xmlns="http://www.w3.org/1999/xhtml">
>>     <BLANKLINE>
>>@@ -307,6 +313,9 @@
>>     <BLANKLINE>
>>               <div class="yui-b" dir="ltr">
>>                 <div>
>>+                  <div class="registering">
>>+                    <div />
>>+                  </div>
>>     <BLANKLINE>
>>                   <ol class="breadcrumbs" xmlns="http://www.w3.org/1999/xhtml">
>>     <BLANKLINE>
>>@@ -463,6 +472,34 @@
>>     <h1>Heading</h1>
>>
>>
>>+Page registering
>>+----------------
>>+
>>+The 'registering' slot is presented on the right side of the 'heading'
>>+and can be filled with the context registering information (registrant
>>+and date_created, normally).
>>+
>>+    >>> class RegisteringView(LaunchpadView):
>>+    ...     """A simple view to test base-layout."""
>>+    ...     __launchpad_facetname__ = 'overview'
>>+    ...     template = ViewPageTemplateFile('testfiles/main-registering.pt')
>>+    ...     page_title = 'Test base-layout: registering'
>
>
> What is the purpose of this page_title variable that doesn't appear to be used
> anywhere?

Nah, bad copy & paste. Fixed, thanks for spotting it.

>>+
>>+    >>> view = RegisteringView(user, request)
>>+    >>> html = view.render()
>>+
>>+    >>> from canonical.launchpad.testing.pages import (
>>+    ...     first_tag_by_class)
>>+
>>+    >>> print first_tag_by_class(html, 'registering')
>>+    <div class="registering">
>>+    <p>something nice about the context registering.</p>
>>+    </div>
>>+
>>+Note that the slot itself will be already 'styled' and it rarely has
>>+to be changed.
>>+
>>+
>> Public and private presentation
>> -------------------------------
>>
>>
>>=== added file 'lib/lp/app/browser/tests/testfiles/main-registering.pt'
>>--- lib/lp/app/browser/tests/testfiles/main-registering.pt     1970-01-01 00:00:00 +0000
>>+++ lib/lp/app/browser/tests/testfiles/main-registering.pt     2009-09-16 20:19:40 +0000
>>@@ -0,0 +1,19 @@
>>+<html
>>+  xmlns="http://www.w3.org/1999/xhtml"
>>+  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_only">
>>+
>>+  <body>
>>+
>>+    <tal:registering metal:fill-slot="registering">
>>+      <p>something nice about the context registering.</p>
>>+    </tal:registering>
>>+
>>+    <tal:main metal:fill-slot="main">
>>+     ANYTHING
>>+    </tal:main>
>>+
>>+  </body>
>>+</html>
>>
>>=== modified file 'lib/lp/soyuz/templates/build-index.pt'
>>--- lib/lp/soyuz/templates/build-index.pt      2009-09-16 12:31:06 +0000
>>+++ lib/lp/soyuz/templates/build-index.pt      2009-09-16 20:19:40 +0000
>>@@ -10,19 +10,18 @@
>>   <body>
>>
>>     <div metal:fill-slot="heading">
>>-      <div style="float: left;">
>>       <h1 tal:content="CONTEXTS/fmt:pagetitle">i386 build of
>>         alsa-utils 1.0.9a-4ubuntu1 in ubuntu hoary RELEASE</h1>
>>-      </div>
>>-      <div style="float: right;">
>>-      <p class="discreet">
>>-        Created <span
>>-          tal:attributes="title context/datecreated/fmt:datetime"
>>-          tal:content="context/datecreated/fmt:displaydate"
>>-          >2005-10-05 5:45</span>
>>+    </div> <!-- heading -->
>
>
>
> CONTEXTS/fmt:pagetitle is dependent on lib/c/l/pagetitles.py, which we
> want to get rid of. The preferred way to move the <h1> out of the template
> is to add a view.label attribute. Since, the BuildView is shared by both
> build-index.pt and build-retry.pt, subclasses would need to be created to
> provide them with the distinct label properties that match the build_index
> and build_retry variables, which need to be removed from lib/c/l/pagetitles.py.

Good point, it seems like a good opportunity the get this fixed. Done.

Thanks, Edwin.
[]
--
Celso Providelo <email address hidden>
IRC: cprov, Jabber: <email address hidden>, Skype: cprovidelo
1024D/681B6469 C858 2652 1A6E F6A6 037B B3F7 9FF2 583E 681B 6469

« Back to merge proposal