Launchpad itself

Merge lp://staging/~edwin-grubbs/launchpad/bug-421967-karma-ui-3 into lp://staging/launchpad

  1. bug-421967-karma-ui-3
  2. Merge into devel
Proposed by Edwin Grubbs
Status: Merged
Approved by: Graham Binns
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp://staging/~edwin-grubbs/launchpad/bug-421967-karma-ui-3
Merge into: lp://staging/launchpad
Diff against target: None lines
To merge this branch: bzr merge lp://staging/~edwin-grubbs/launchpad/bug-421967-karma-ui-3
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Curtis Hovey (community) ui Approve
Barry Warsaw (community) ui* Approve
Review via email: mp+10956@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

Summary
-------

Converted the /karmaaction page and the $pillar/+topcontributors to UI 3.0.

Implementation details
----------------------

In addition to the fairly mechanical changes, I made the topcontributors
page look less ugly and confusing.

Tests
-----

./bin/test -vv -t karma

Demo and Q/A
------------

* Open https://launchpad.dev/ubuntu/+topcontributors
* Open https://launchpad.dev/karmaaction

Revision history for this message
Graham Binns (gmb) wrote :

Hi Edwin,

Thanks for this branch. Your change looks good overall but I've issues
with the inline stylesheet. See below for details.

> === modified file 'lib/lp/registry/templates/karmaaction-index.pt'
> --- lib/lp/registry/templates/karmaaction-index.pt 2009-07-17 17:59:07 +0000
> +++ lib/lp/registry/templates/karmaaction-index.pt 2009-09-01 04:08:55 +0000
> @@ -6,17 +6,18 @@
> xml:lang="en"
> lang="en"
> dir="ltr"
> - metal:use-macro="context/@@main_template/master"
> + metal:use-macro="view/macro:page/main_only"
> i18n:domain="launchpad"
> >
> - <body>
> - <metal:heading fill-slot="pageheading">
> - <h1>Actions that give people karma</h1>
> - </metal:heading>
> +<body>
> +
> +<metal:heading fill-slot="heading">
> + <h1 tal:content="view/page_title" />
> +</metal:heading>
> +
>
> <div metal:fill-slot="main">
>
> -
> <table class="listing sortable" id="karmas">
> <thead>
> <tr>
>
> === modified file 'lib/lp/registry/templates/karmacontext-topcontributors.pt'
> --- lib/lp/registry/templates/karmacontext-topcontributors.pt 2009-07-17 17:59:07 +0000
> +++ lib/lp/registry/templates/karmacontext-topcontributors.pt 2009-09-01 04:08:55 +0000
> @@ -6,51 +6,76 @@
> xml:lang="en"
> lang="en"
> dir="ltr"
> - metal:use-macro="context/@@main_template/master"
> + metal:use-macro="view/macro:page/main_only"
> i18n:domain="launchpad"
> >
> - <body>
> +<body>
> +
> +<h1 metal:fill-slot="heading" tal:content="view/page_title" />
>
> <div metal:fill-slot="main">
>
> - <h1>Top contributors</h1>
> <p>
> Here you can see the people who have earned most Launchpad
> - karma while working on <span id="project-name" tal:content="context/displayname">this project</span>.
> + karma while working on
> + <span id="project-name" tal:content="context/displayname">
> + this project
> + </span>.
> (<a href="https://help.launchpad.net/YourAccount/Karma">What is karma?</a>)
> </p>
> +
> + <style>
> + table {
> + max-width: 43em;
> + }
> + thead th.karma-person {
> + width: 10em;
> + }
> + thead th.karma-count {
> + width: 16em;
> + }
> + tbody td {
> + white-space: nowrap;
> + }
> + </style>
> +

I'm not really happy with this block being here. Traditionally we've
always put styles in style.css; why not this one, too? Just make it
table.karma or something like that.

If there's a good reason for leaving this here, the <style> tag needs a
type="text/css" declaration.

review: Approve
Revision history for this message
Graham Binns (gmb) wrote :

Previous review should've been Needs Fixing. Sorry.

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

Thanks for these change Edwin.

Like Graham, I do not think this use of a style sheet is appropriate, or successful. The use of nowrap causes the tables to be different sizes in my browser.

Consider using the style attribute or a <col width="">. Since this

if you must do this. '43em' is an arbitrary width. style-3.0.css uses 45em which is an established length for reading English lines. I think your intent here is to say that this table is a part of the narrative so should be the same width. I think that is a valid rule and you can create a class in style-3.0.css (.narrative {width: 45em;}) that you can use.

review: Needs Fixing (ui)
Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

One line change to fix the xx-karmacontext-topcontributors.txt test.

{{{
=== modified file 'lib/lp/registry/browser/karma.py'
--- lib/lp/registry/browser/karma.py 2009-09-01 04:08:55 +0000
+++ lib/lp/registry/browser/karma.py 2009-09-01 13:44:08 +0000
@@ -78,7 +78,7 @@

     @property
     def page_title(self):
- return "Top %s contributors" % self.context.displayname
+ return "Top %s Contributors" % self.context.title

     def initialize(self):
         context = self.context

}}}

Revision history for this message
Barry Warsaw (barry) wrote :

On Sep 01, 2009, at 04:20 AM, Edwin Grubbs wrote:

>Demo and Q/A
>------------
>
>* Open https://launchpad.dev/ubuntu/+topcontributors
>* Open https://launchpad.dev/karmaaction

I mentioned the following issues in irc.

* The karma values don't look good jammed all the way to the right in long
  columns like "Specification Tracking Karma". I suggested to center those
  values but you didn't think it would look good with long values not lining
  up under shorter values. You suggested to increase the padding-right to
  something like 30%. That sounded good to me; see how it looks and use your
  own judgment on it.

* The special <h1>'s should be removed from the heading slot. This will give
  you some extra headers, but that's a global problem I intend to fix today.

* There's a grammatical mistake on +topcontributors:

  "Here you can see the people who have earned most Launchpad karma while
   working on Ubuntu. (What is karma?)"

   To fix: s/earned most/earned the most/

With those changes:

 review approve ui*

review: Approve (ui*)
Revision history for this message
Curtis Hovey (sinzui) wrote :

Thanks for fixing these issues Edwin. The page looks good to land.

review: Approve (ui)
Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :
Download full text (5.6 KiB)

> Hi Edwin,
>
> Thanks for this branch. Your change looks good overall but I've issues
> with the inline stylesheet. See below for details.
>
> > === modified file 'lib/lp/registry/templates/karmaaction-index.pt'
> > --- lib/lp/registry/templates/karmaaction-index.pt 2009-07-17 17:59:07
> +0000
> > +++ lib/lp/registry/templates/karmaaction-index.pt 2009-09-01 04:08:55
> +0000
> > @@ -6,17 +6,18 @@
> > xml:lang="en"
> > lang="en"
> > dir="ltr"
> > - metal:use-macro="context/@@main_template/master"
> > + metal:use-macro="view/macro:page/main_only"
> > i18n:domain="launchpad"
> > >
> > - <body>
> > - <metal:heading fill-slot="pageheading">
> > - <h1>Actions that give people karma</h1>
> > - </metal:heading>
> > +<body>
> > +
> > +<metal:heading fill-slot="heading">
> > + <h1 tal:content="view/page_title" />
> > +</metal:heading>
> > +
> >
> > <div metal:fill-slot="main">
> >
> > -
> > <table class="listing sortable" id="karmas">
> > <thead>
> > <tr>
> >
> > === modified file 'lib/lp/registry/templates/karmacontext-
> topcontributors.pt'
> > --- lib/lp/registry/templates/karmacontext-topcontributors.pt 2009-07-17
> 17:59:07 +0000
> > +++ lib/lp/registry/templates/karmacontext-topcontributors.pt 2009-09-01
> 04:08:55 +0000
> > @@ -6,51 +6,76 @@
> > xml:lang="en"
> > lang="en"
> > dir="ltr"
> > - metal:use-macro="context/@@main_template/master"
> > + metal:use-macro="view/macro:page/main_only"
> > i18n:domain="launchpad"
> > >
> > - <body>
> > +<body>
> > +
> > +<h1 metal:fill-slot="heading" tal:content="view/page_title" />
> >
> > <div metal:fill-slot="main">
> >
> > - <h1>Top contributors</h1>
> > <p>
> > Here you can see the people who have earned most Launchpad
> > - karma while working on <span id="project-name"
> tal:content="context/displayname">this project</span>.
> > + karma while working on
> > + <span id="project-name" tal:content="context/displayname">
> > + this project
> > + </span>.
> > (<a href="https://help.launchpad.net/YourAccount/Karma">What is
> karma?</a>)
> > </p>
> > +
> > + <style>
> > + table {
> > + max-width: 43em;
> > + }
> > + thead th.karma-person {
> > + width: 10em;
> > + }
> > + thead th.karma-count {
> > + width: 16em;
> > + }
> > + tbody td {
> > + white-space: nowrap;
> > + }
> > + </style>
> > +
>
> I'm not really happy with this block being here. Traditionally we've
> always put styles in style.css; why not this one, too? Just make it
> table.karma or something like that.
>
> If there's a good reason for leaving this here, the <style> tag needs a
> type="text/css" declaration.

Hi Graham,

Thanks for the review.

I got rid of the inline <style> block. Some of it, I moved into the table.narrow-listing css class, and the rest of it I set with the style attribute as sinzui suggested. I also removed the fill-slot="heading" and centered the karma numbers as barry suggested.

-Edwin

{{{
=== modified file 'lib/canonical/launchpad/icing/style-3-0.css'
--- lib/canonical/launchpad/icing/style-3-0.css 2009-08-31 13:12:03 +0000
+++ lib/canonical/launchpad/icing/style-3-0....

Read more...

Revision history for this message
Graham Binns (gmb) wrote :

Hi Edwin,

Thanks for fixing this. r=me.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/pagetitles.py'
2--- lib/canonical/launchpad/pagetitles.py 2009-08-31 22:11:14 +0000
3+++ lib/canonical/launchpad/pagetitles.py 2009-09-01 04:08:55 +0000
4@@ -514,10 +514,6 @@
5 hwdb_submit_hardware_data = (
6 'Submit New Data to the Launchpad Hardware Database')
7
8-karmaaction_index = 'Karma actions'
9-
10-karmacontext_topcontributors = ContextTitle('Top %s Contributors')
11-
12 language_index = ContextDisplayName("%s in Launchpad")
13
14 languageset_index = 'Languages in Launchpad'
15
16=== modified file 'lib/lp/registry/browser/configure.zcml'
17--- lib/lp/registry/browser/configure.zcml 2009-09-01 00:21:10 +0000
18+++ lib/lp/registry/browser/configure.zcml 2009-09-01 04:08:55 +0000
19@@ -542,6 +542,7 @@
20 for="lp.registry.interfaces.karma.IKarmaActionSet"
21 name="+index"
22 permission="launchpad.Admin"
23+ class="lp.registry.browser.karma.KarmaActionView"
24 template="../templates/karmaaction-index.pt"/>
25 <browser:pages
26 for="lp.registry.interfaces.karma.IKarmaContext"
27
28=== modified file 'lib/lp/registry/browser/karma.py'
29--- lib/lp/registry/browser/karma.py 2009-08-16 17:02:13 +0000
30+++ lib/lp/registry/browser/karma.py 2009-09-01 04:08:55 +0000
31@@ -34,6 +34,12 @@
32 return self.context.getByName(name)
33
34
35+class KarmaActionView(LaunchpadView):
36+ """View class for the index of karma actions."""
37+
38+ page_title = 'Actions that give people karma'
39+
40+
41 class KarmaActionEditView(LaunchpadEditFormView):
42
43 schema = IKarmaAction
44@@ -70,6 +76,10 @@
45 class KarmaContextTopContributorsView(LaunchpadView):
46 """List this KarmaContext's top contributors."""
47
48+ @property
49+ def page_title(self):
50+ return "Top %s contributors" % self.context.displayname
51+
52 def initialize(self):
53 context = self.context
54 if IProduct.providedBy(context):
55
56=== modified file 'lib/lp/registry/templates/karmaaction-index.pt'
57--- lib/lp/registry/templates/karmaaction-index.pt 2009-07-17 17:59:07 +0000
58+++ lib/lp/registry/templates/karmaaction-index.pt 2009-09-01 04:08:55 +0000
59@@ -6,17 +6,18 @@
60 xml:lang="en"
61 lang="en"
62 dir="ltr"
63- metal:use-macro="context/@@main_template/master"
64+ metal:use-macro="view/macro:page/main_only"
65 i18n:domain="launchpad"
66 >
67- <body>
68- <metal:heading fill-slot="pageheading">
69- <h1>Actions that give people karma</h1>
70- </metal:heading>
71+<body>
72+
73+<metal:heading fill-slot="heading">
74+ <h1 tal:content="view/page_title" />
75+</metal:heading>
76+
77
78 <div metal:fill-slot="main">
79
80-
81 <table class="listing sortable" id="karmas">
82 <thead>
83 <tr>
84
85=== modified file 'lib/lp/registry/templates/karmacontext-topcontributors.pt'
86--- lib/lp/registry/templates/karmacontext-topcontributors.pt 2009-07-17 17:59:07 +0000
87+++ lib/lp/registry/templates/karmacontext-topcontributors.pt 2009-09-01 04:08:55 +0000
88@@ -6,51 +6,76 @@
89 xml:lang="en"
90 lang="en"
91 dir="ltr"
92- metal:use-macro="context/@@main_template/master"
93+ metal:use-macro="view/macro:page/main_only"
94 i18n:domain="launchpad"
95 >
96- <body>
97+<body>
98+
99+<h1 metal:fill-slot="heading" tal:content="view/page_title" />
100
101 <div metal:fill-slot="main">
102
103- <h1>Top contributors</h1>
104 <p>
105 Here you can see the people who have earned most Launchpad
106- karma while working on <span id="project-name" tal:content="context/displayname">this project</span>.
107+ karma while working on
108+ <span id="project-name" tal:content="context/displayname">
109+ this project
110+ </span>.
111 (<a href="https://help.launchpad.net/YourAccount/Karma">What is karma?</a>)
112 </p>
113+
114+ <style>
115+ table {
116+ max-width: 43em;
117+ }
118+ thead th.karma-person {
119+ width: 10em;
120+ }
121+ thead th.karma-count {
122+ width: 16em;
123+ }
124+ tbody td {
125+ white-space: nowrap;
126+ }
127+ </style>
128+
129 <h2>Overall</h2>
130
131 <table class="listing sortable" id="overall_top">
132 <thead>
133 <tr>
134- <th>Person</th>
135- <th><span tal:replace="view/context_name" /> Karma</th>
136- <th>Total Karma</th>
137+ <th class="karma-person">Person</th>
138+ <th class="karma-count">
139+ <span tal:replace="view/context_name" /> Karma
140+ </th>
141+ <th class="karma-total">Total Karma</th>
142 </tr>
143 </thead>
144 <tbody tal:define="contributors view/getTopContributors">
145- <metal:contributors
146+ <metal:contributors
147 use-macro="context/@@karmacontext-macros/top-contributors-table-body" />
148 </tbody>
149 </table>
150
151- <h2>By category</h2>
152-
153- <tal:block
154+
155+ <h2 style="margin-top: 2em">By category</h2>
156+
157+ <tal:block
158 define="contributors_by_category view/top_contributors_by_category">
159 <tal:categories repeat="category view/sorted_categories">
160- <h3 tal:content="category" />
161- <table class="listing sortable" tal:attributes="id category">
162+ <table style="margin-bottom: 2em"
163+ class="listing sortable"
164+ tal:attributes="id category"
165+ >
166 <thead>
167 <tr>
168- <th>Person</th>
169- <th><span tal:replace="category" /> Karma</th>
170- <th>Total Karma</th>
171+ <th class="karma-person">Person</th>
172+ <th class="karma-count"><span tal:replace="category" /> Karma</th>
173+ <th class="karma-total">Total Karma</th>
174 </tr>
175 </thead>
176 <tbody tal:define="contributors contributors_by_category/?category">
177- <metal:contributors
178+ <metal:contributors
179 use-macro="context/@@karmacontext-macros/top-contributors-table-body" />
180 </tbody>
181 </table>
182@@ -61,4 +86,3 @@
183
184 </body>
185 </html>
186-<!-- 1-0 done -->