Launchpad itself

Merge lp:~henninge/launchpad/bug-467079 into lp:launchpad

  1. bug-467079
  2. Merge into devel
Proposed by Henning Eggers
Status: Merged
Approved by: Henning Eggers
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~henninge/launchpad/bug-467079
Merge into: lp:launchpad
Diff against target: 152 lines (+27/-21)
4 files modified
lib/canonical/launchpad/icing/style-3-0.css (+6/-1)
lib/canonical/launchpad/templates/root-index.pt (+5/-5)
lib/lp/registry/browser/root.py (+1/-6)
lib/lp/registry/stories/launchpad-root/xx-featuredprojects.txt (+15/-9)
To merge this branch: bzr merge lp:~henninge/launchpad/bug-467079
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Michael Nelson (community) release-critical Abstain
Paul Hummer (community) code Approve
Review via email: mp+14264@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Henning Eggers (henninge) wrote :

= Bug 467079 =

The list of featured projects on the home page had a fixed length, an error which I introduced last cylcle although the old solution was bad too. It capped the first column at a fixed number and let the second grow to infinivity.

== Proposed fix ==

Dynamically calculate the length for each column according to the number of featured projects minus the top featured project. If the number is odd, make the first column one longer.

== Text ==

bin/test -vvct featuredprojects

== Demo/QA ==

Look at the homepage and see if Ubuntu and Zope are visible again ...

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/registry/browser/root.py
  lib/lp/registry/stories/launchpad-root/xx-featuredprojects.txt

== Pylint notices ==

lib/lp/registry/browser/root.py
    45: [F0401] Unable to import 'lazr.batchnavigator.z3batching' (No module named batchnavigator)
    509: [E1002, WindowedListBatch.__iter__] Use super on an old style class

Revision history for this message
Paul Hummer (rockstar) :
review: Approve (code)
Revision history for this message
Michael Nelson (michael.nelson) wrote :

Hi Henning,

Thanks for fixing this! I'd prefer to see this land on edge next week - I can't classify it as a blocker, but I won't be offended if you get a second opinion :)

review: Abstain (release-critical)
Revision history for this message
Curtis Hovey (sinzui) wrote :
Download full text (3.9 KiB)

Hi Henning.

  review needs-fixing

Your view and CSS changes are good. The narrative of the story needs
revising. It is not a story. The narrative must explain what is seen and
happens from the users perspective. *We* care about contracts, and if
this test ware really about what *we* want to create, we could
instantiate the view, call render() and verify the markup much easier in
a view test.

I do not see how this test can pass. It checks for ids that do not exist
since the columns table-column properties were replace with simple CSS.

On Tue, 2009-11-17 at 17:23 +0000, Henning Eggers wrote:
> === modified file 'lib/lp/registry/stories/launchpad-root/xx-featuredprojects.txt'
> --- lib/lp/registry/stories/launchpad-root/xx-featuredprojects.txt 2009-09-21 14:48:17 +0000
> +++ lib/lp/registry/stories/launchpad-root/xx-featuredprojects.txt 2009-11-17 17:21:11 +0000
> @@ -24,21 +24,32 @@
> >>> print extract_text(featured.h2)
> Featured projects
>
> -We show the featured projects in our sample data, the "project of the day" is
> -"GNOME" and it is listed first:
> +We show the featured projects in our sample data. The "project of the day"
> +is "GNOME" and it is listed separately:

Any user can see the Launchpad featured projects. The project of the day
is listed separately.

> - >>> for link in featured.findAll('a'):
> - ... print extract_text(link)
> + >>> top_project = find_tag_by_id(featured, 'homepage-featured-top-title')
> + >>> print extract_text(top_project)
> GNOME
> +
> +These are the projects in the left column.

He see's content in a left column.

--but I cannot see how this will work because the template does not have
these ids, we let the stylesheet do the layout. There is a single list
that we need to test. If the layout is changed in the CSS, we do not
need to worry about the test breaking. This was one of the 3.0 UI
goals--no brittle tests that verifies markup. While this does not verify
the markup, it still feels like it is trying to force a two column
table.

> + >>> def print_column(column_id):
> + ... column = find_tag_by_id(featured, column_id)
> + ... for link in column.findAll('a'):
> + ... print extract_text(link)
> + >>> print_column('homepage-featured-left')
> Gnome Applets
> Bazaar
> Mozilla Firefox
> Gentoo
> +
> +These are the projects in the right column.

And content in a right column.

-- but I know that this test will fail

> + >>> print_column('homepage-featured-right')
> GNOME Terminal
> the Mozilla Project
> Mozilla Thunderbird
> Ubuntu
> - Browse all ... projects
>
> == Adding a featured project ==
>
> @@ -88,19 +99,27 @@
>
> >>> anon_browser.open('http://launchpad.dev/')
> >>> featured = find_tag_by_id(anon_browser.contents, 'homepage-featured')
> - >>> for link in featured.findAll('a'):
> - ... print extract_text(link)
> + >>> top_project = find_tag_by_id(featured, 'homepage-featured-top-title')
> + >>> print extract_text(top_project)
> Gentoo

I am not sure 'odd' is important here if we change the css, to be 3
column or 1 column it will not be odd. As...

Read more...

review: Needs Fixing
Revision history for this message
Henning Eggers (henninge) wrote :

Am 18.11.2009 01:18, Curtis Hovey schrieb:
> Review: Needs Fixing
> Hi Henning.
>
> review needs-fixing
>
> Your view and CSS changes are good.

Thanks, great to hear.

> The narrative of the story needs
> revising. It is not a story. The narrative must explain what is seen and
> happens from the users perspective. *We* care about contracts, and if
> this test ware really about what *we* want to create, we could
> instantiate the view, call render() and verify the markup much easier in
> a view test.

Ok, I reworded the test in that manner.

>
> I do not see how this test can pass. It checks for ids that do not exist
> since the columns table-column properties were replace with simple CSS.

I am very sorry but I knew that and I thought I had told you about it on
IRC yesterday. I just didn't have the time to update the test. This test
was still referring to the changes I had originally introduced. Again,
sorry for getting you confused...

I fixed the the test and also changed an id in the template to a class
because the id was used twice. I'll paste the incremental diff.

Cheers,
Henning

Revision history for this message
Henning Eggers (henninge) wrote :
Download full text (4.3 KiB)

=== modified file 'lib/canonical/launchpad/templates/root-index.pt'
--- lib/canonical/launchpad/templates/root-index.pt 2009-11-13 18:03:35 +0000
+++ lib/canonical/launchpad/templates/root-index.pt 2009-11-18 11:03:37 +0000
@@ -51,14 +51,14 @@
           margin-top: 1em;
           font-size: 161%;
           }
- .featured-project h3 {
+ .featured-project-top h3 {
           font-size: 131%;
           font-weight: bold;
           }
- .featured-project h3 img {
+ .featured-project-top h3 img {
           vertical-align: middle;
           }
- .featured-project p {
+ .featured-project-top p {
           margin-top: 0.5em;
           margin-bottom: 1em;
           border-bottom: 1px solid #888888;
@@ -226,7 +226,7 @@
             <div id="homepage-featured" class="homepage-portlet">
               <h2>Featured projects</h2>

- <div class="featured-project"
+ <div class="featured-project-top"
                    tal:define="topproject view/featured_projects_top"
                    tal:condition="topproject">
                 <h3>
@@ -241,7 +241,7 @@
               </div>

               <div class="two-column-list">
- <ul id="homepage-featured">
+ <ul class="featured-projects-list">
                   <li tal:repeat="project view/featured_projects">
                     <a tal:replace="structure project/fmt:link" />
                   </li>

=== modified file 'lib/lp/registry/stories/launchpad-root/xx-featuredprojects.txt'
--- lib/lp/registry/stories/launchpad-root/xx-featuredprojects.txt 2009-10-31 18:32:05 +0000
+++ lib/lp/registry/stories/launchpad-root/xx-featuredprojects.txt 2009-11-18 11:22:40 +0000
@@ -19,33 +19,26 @@
     >>> from lp.registry.browser.root import LaunchpadRootIndexView
     >>> LaunchpadRootIndexView._get_day_of_year = staticmethod(
     ... fake_get_day_of_year)
+
+Anonymous users will see the list of featured projects with links to the
+projects' pages in Launchpad. The "project of the day" is listed separately.
+
     >>> anon_browser.open('http://launchpad.dev/')
     >>> featured = find_tag_by_id(anon_browser.contents, 'homepage-featured')
     >>> print extract_text(featured.h2)
     Featured projects

-We show the featured projects in our sample data. The "project of the day"
-is "GNOME" and it is listed separately:
-
- >>> top_project = find_tag_by_id(featured, 'homepage-featured-top-title')
- >>> print extract_text(top_project)
+ >>> top_project = featured.find('', 'featured-project-top')
+ >>> print extract_text(top_project.h3)
     GNOME

-These are the projects in the left column.
-
- >>> def print_column(column_id):
- ... column = find_tag_by_id(featured, column_id)
- ... for link in column.findAll('a'):
- ... print extract_text(link)
- >>> print_column('homepage-featured-left')
+ >>> featured_list = featured.find('', 'featured-projects-list')
+ >>> for link in featured_list.findAll('a'):
+ ... print extract_text(link)
     Gnome Applets
     Bazaar
     Mozilla Firefox
     Gentoo
-
-These are the projects in the ri...

Read more...

Revision history for this message
Curtis Hovey (sinzui) wrote :

Thanks for removing the limitiation

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/launchpad/icing/style-3-0.css'
--- lib/canonical/launchpad/icing/style-3-0.css 2009-11-13 17:49:40 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css 2009-11-18 11:26:12 +0000
@@ -471,13 +471,18 @@
471ol.subordinate {471ol.subordinate {
472 margin-left: 4em;472 margin-left: 4em;
473 }473 }
474.two-column-list li,
475.two-column-list dl {474.two-column-list dl {
476 width: 48%;475 width: 48%;
477 float: left;476 float: left;
478 display: inline;477 display: inline;
479 margin: 0 0.25em 0.75em 0;478 margin: 0 0.25em 0.75em 0;
480 }479 }
480.two-column-list li {
481 width: 48%;
482 float: left;
483 display: inline;
484 margin: 0 0.25em 0 0;
485 }
481.two-column-list:after {486.two-column-list:after {
482 content: ".";487 content: ".";
483 display: block;488 display: block;
484489
=== modified file 'lib/canonical/launchpad/templates/root-index.pt'
--- lib/canonical/launchpad/templates/root-index.pt 2009-11-13 18:03:35 +0000
+++ lib/canonical/launchpad/templates/root-index.pt 2009-11-18 11:26:12 +0000
@@ -51,14 +51,14 @@
51 margin-top: 1em;51 margin-top: 1em;
52 font-size: 161%;52 font-size: 161%;
53 }53 }
54 .featured-project h3 {54 .featured-project-top h3 {
55 font-size: 131%;55 font-size: 131%;
56 font-weight: bold;56 font-weight: bold;
57 }57 }
58 .featured-project h3 img {58 .featured-project-top h3 img {
59 vertical-align: middle;59 vertical-align: middle;
60 }60 }
61 .featured-project p {61 .featured-project-top p {
62 margin-top: 0.5em;62 margin-top: 0.5em;
63 margin-bottom: 1em;63 margin-bottom: 1em;
64 border-bottom: 1px solid #888888;64 border-bottom: 1px solid #888888;
@@ -226,7 +226,7 @@
226 <div id="homepage-featured" class="homepage-portlet">226 <div id="homepage-featured" class="homepage-portlet">
227 <h2>Featured projects</h2>227 <h2>Featured projects</h2>
228228
229 <div class="featured-project"229 <div class="featured-project-top"
230 tal:define="topproject view/featured_projects_top"230 tal:define="topproject view/featured_projects_top"
231 tal:condition="topproject">231 tal:condition="topproject">
232 <h3>232 <h3>
@@ -241,7 +241,7 @@
241 </div>241 </div>
242242
243 <div class="two-column-list">243 <div class="two-column-list">
244 <ul id="homepage-featured">244 <ul class="featured-projects-list">
245 <li tal:repeat="project view/featured_projects">245 <li tal:repeat="project view/featured_projects">
246 <a tal:replace="structure project/fmt:link" />246 <a tal:replace="structure project/fmt:link" />
247 </li>247 </li>
248248
=== modified file 'lib/lp/registry/browser/root.py'
--- lib/lp/registry/browser/root.py 2009-11-13 18:23:59 +0000
+++ lib/lp/registry/browser/root.py 2009-11-18 11:26:12 +0000
@@ -54,10 +54,6 @@
54class LaunchpadRootIndexView(HasAnnouncementsView, LaunchpadView):54class LaunchpadRootIndexView(HasAnnouncementsView, LaunchpadView):
55 """An view for the default view of the LaunchpadRoot."""55 """An view for the default view of the LaunchpadRoot."""
5656
57 # The homepage has two columns to hold featured projects. This
58 # determines the number of projects we display in each column.
59 FEATURED_PROJECT_ROWS = 11
60
61 featured_projects = []57 featured_projects = []
62 featured_projects_top = None58 featured_projects_top = None
6359
@@ -75,9 +71,8 @@
75 super(LaunchpadRootIndexView, self).initialize()71 super(LaunchpadRootIndexView, self).initialize()
76 # The maximum number of projects to be displayed as defined by the72 # The maximum number of projects to be displayed as defined by the
77 # number of items plus one top featured project.73 # number of items plus one top featured project.
78 max_projects = self.FEATURED_PROJECT_ROWS + 1
79 self.featured_projects = list(74 self.featured_projects = list(
80 getUtility(IPillarNameSet).featured_projects)[:max_projects]75 getUtility(IPillarNameSet).featured_projects)
81 self._setFeaturedProjectsTop()76 self._setFeaturedProjectsTop()
8277
83 def _setFeaturedProjectsTop(self):78 def _setFeaturedProjectsTop(self):
8479
=== modified file 'lib/lp/registry/stories/launchpad-root/xx-featuredprojects.txt'
--- lib/lp/registry/stories/launchpad-root/xx-featuredprojects.txt 2009-09-21 14:48:17 +0000
+++ lib/lp/registry/stories/launchpad-root/xx-featuredprojects.txt 2009-11-18 11:26:12 +0000
@@ -19,17 +19,22 @@
19 >>> from lp.registry.browser.root import LaunchpadRootIndexView19 >>> from lp.registry.browser.root import LaunchpadRootIndexView
20 >>> LaunchpadRootIndexView._get_day_of_year = staticmethod(20 >>> LaunchpadRootIndexView._get_day_of_year = staticmethod(
21 ... fake_get_day_of_year)21 ... fake_get_day_of_year)
22
23Anonymous users will see the list of featured projects with links to the
24projects' pages in Launchpad. The "project of the day" is listed separately.
25
22 >>> anon_browser.open('http://launchpad.dev/')26 >>> anon_browser.open('http://launchpad.dev/')
23 >>> featured = find_tag_by_id(anon_browser.contents, 'homepage-featured')27 >>> featured = find_tag_by_id(anon_browser.contents, 'homepage-featured')
24 >>> print extract_text(featured.h2)28 >>> print extract_text(featured.h2)
25 Featured projects29 Featured projects
2630
27We show the featured projects in our sample data, the "project of the day" is31 >>> top_project = featured.find('', 'featured-project-top')
28"GNOME" and it is listed first:32 >>> print extract_text(top_project.h3)
33 GNOME
2934
30 >>> for link in featured.findAll('a'):35 >>> featured_list = featured.find('', 'featured-projects-list')
36 >>> for link in featured_list.findAll('a'):
31 ... print extract_text(link)37 ... print extract_text(link)
32 GNOME
33 Gnome Applets38 Gnome Applets
34 Bazaar39 Bazaar
35 Mozilla Firefox40 Mozilla Firefox
@@ -38,7 +43,6 @@
38 the Mozilla Project43 the Mozilla Project
39 Mozilla Thunderbird44 Mozilla Thunderbird
40 Ubuntu45 Ubuntu
41 Browse all ... projects
4246
43== Adding a featured project ==47== Adding a featured project ==
4448
@@ -88,9 +92,13 @@
8892
89 >>> anon_browser.open('http://launchpad.dev/')93 >>> anon_browser.open('http://launchpad.dev/')
90 >>> featured = find_tag_by_id(anon_browser.contents, 'homepage-featured')94 >>> featured = find_tag_by_id(anon_browser.contents, 'homepage-featured')
91 >>> for link in featured.findAll('a'):95 >>> top_project = featured.find('', 'featured-project-top')
96 >>> print extract_text(top_project.h3)
97 Gentoo
98
99 >>> featured_list = featured.find('', 'featured-projects-list')
100 >>> for link in featured_list.findAll('a'):
92 ... print extract_text(link)101 ... print extract_text(link)
93 Gentoo
94 Apache102 Apache
95 Gnome Applets103 Gnome Applets
96 Bazaar104 Bazaar
@@ -100,8 +108,6 @@
100 the Mozilla Project108 the Mozilla Project
101 Mozilla Thunderbird109 Mozilla Thunderbird
102 Ubuntu110 Ubuntu
103 Browse all ... projects
104
105111
106== Removing a project ==112== Removing a project ==
107113