Launchpad itself

Merge lp:~bac/launchpad/bug-461164-downloadfix into lp:launchpad

  1. bug-461164-downloadfix
  2. Merge into devel
Proposed by Brad Crittenden
Status: Merged
Merged at revision: not available
Proposed branch: lp:~bac/launchpad/bug-461164-downloadfix
Merge into: lp:launchpad
Diff against target: 205 lines
1 file modified
lib/lp/registry/templates/product-files.pt (+90/-87)
To merge this branch: bzr merge lp:~bac/launchpad/bug-461164-downloadfix
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+13969@code.launchpad.net

Commit message

Clean up the +download page template to avoid generating extra vertical space. Also make changes to generate valid CSS identifiers.

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

= Summary =

The +download page was creating extra vertical space. Some class ids were not proper
 CSS as they were not unique and had _ instead of -.

== Proposed fix ==

Re-arranged some of the conditional elements to not generate unnecessary <p> and
other elements. Used fmt:css-id to ensure proper ids.

== Pre-implementation notes ==

Quick chat with Curtis.

== Implementation details ==

As above.

== Tests ==

No tests were changed. I'm running the full registry suite to ensure nothing has
been broken.

== Demo and Q/A ==

Go to https://launchpad.dev/firefox. Create some extra series with no releases.
View https://launchpad.dev/firefox/+download and ensure there is not extra space at
the bottom.

Inspect the generated HTML to ensure there are no invalid CSS elements.

= 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/templates/product-files.pt

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

On line 74 I see:
    <h2 style="font-size: 1.2em;">

Do you know why this <h2> is an exception? Does it need to be? If it needs to be, is the rule applicable to other pages and should thus be in the CSS?

Also: YUI-Font does not allow fonts to be set with em because IE will display it differently from other browsers. If we need a different font-size, it must be one of the percentages from style-3.0.css.

Can we replace the python expression on line 86
    tal:condition="python: release.release_notes or release.changelog"
with
    tal:condition="release/release_notes|release/changelog|nothing"

I am still concerned about the changelog. I may be in a minority in my belief that it does not belong on this page, but regardless, it is a problem. Changelogs have killed the SP and DSP pages in the past because they are large and require special formatting. I see we are doing that in this page. Look at
    https://edge.launchpad.net/rhythmbox/+download
If you really must keep this information on a page for users who do not care about details, then truncate the field.

When I look at https://edge.launchpad.net/linux/+download I see phantom series that I created from the <div class="portlet-border">. I think you fixed this looking at lines 10 and 14 of the diff. Were you aware of this issue?

review: Needs Information (code)
Revision history for this message
Brad Crittenden (bac) wrote :

Thanks Curtis. I have taken care of all of your issues, except completely killing changelog.

Diff at http://pastebin.ubuntu.com/302205/

Revision history for this message
Brad Crittenden (bac) wrote :

test

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

Hi Brad.

Thanks for addressing all these issues on IRC. The diff looks good. This branch is good to land.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/templates/product-files.pt'
2--- lib/lp/registry/templates/product-files.pt 2009-10-22 15:58:34 +0000
3+++ lib/lp/registry/templates/product-files.pt 2009-10-26 17:10:23 +0000
4@@ -39,111 +39,114 @@
5
6 <div
7 tal:repeat="series view/sorted_series_list"
8- tal:attributes="id string:series_${series/name}">
9-
10- <div tal:condition="not: repeat/series/start" class="portlet-border" />
11+ tal:attributes="id series/name/fmt:css-id/series-">
12
13 <tal:seriesfilesexist condition="series/has_release_files">
14+ <div tal:condition="not: repeat/series/start" class="portlet-border" />
15 <tal:releases repeat="release series/releases">
16 <tal:releasefilesexist condition="release/files">
17- <div tal:condition="not: repeat/release/start" class="portlet-border" />
18- <div tal:condition="release/files" class="top-portlet">
19- <h2 style="font-size: 1.2em; float:left;">
20- <a tal:attributes="href release/fmt:url">
21- <span tal:replace="release/name_with_codename" /> release</a>
22- from the
23- <a tal:attributes="href series/fmt:url"
24- tal:content="series/name">name</a> series released
25- <span
26- tal:attributes="title release/datereleased/fmt:datetime"
27- tal:content="release/datereleased/fmt:approximatedate" />
28- </h2>
29-
30- <div style="clear:both;" />
31-
32- <div id="release_information"
33- tal:condition="python: release.release_notes or release.changelog">
34-
35- <fieldset class="collapsible collapsed" style="padding: 0px;">
36- <legend>Release information</legend>
37-
38- <div tal:condition="release/release_notes">
39- <strong>Release notes:</strong>
40- <div id="release_notes" style="margin-bottom: 0px;"
41- tal:define="notes release/release_notes/fmt:shorten/800"
42- tal:content="structure notes/fmt:text-to-html">
43- ProductRelease.release_notes
44- </div>
45- </div>
46-
47- <div tal:condition="release/changelog">
48- <strong>Changelog:</strong>
49- <div id="changelog" style="margin-bottom: 0px;"
50- tal:content="structure release/changelog/fmt:obfuscate-email/fmt:text-to-html">
51- ProductRelease.changelog.
52- </div>
53- </div>
54- </fieldset>
55- </div> <!-- release information -->
56-
57- <table class="listing" style="margin-top: 1em;">
58- <thead>
59- <tr>
60- <th width="35%">File</th>
61- <th width="45%">Description</th>
62- <th>Downloads</th>
63- <th tal:condition="has_edit">Delete</th>
64- </tr>
65- </thead>
66- <tbody>
67- <tr tal:repeat="file release/files">
68- <tal:define-vars
69- tal:define="checkbox_index string:${repeat/series/index}_${repeat/release/index}_${repeat/file/index}">
70- <tal:release-file
71- metal:use-macro="file/@@+macros/detailed_display" />
72+ <div tal:condition="not: repeat/release/start" class="portlet-border" />
73+ <div class="top-portlet">
74+ <h2 style="font-size: 1.2em;">
75+ <a tal:attributes="href release/fmt:url">
76+ <span tal:replace="release/name_with_codename" /> release</a>
77+ from the
78+ <a tal:attributes="href series/fmt:url"
79+ tal:content="series/name">name</a> series released
80+ <span
81+ tal:attributes="title release/datereleased/fmt:datetime"
82+ tal:content="release/datereleased/fmt:approximatedate" />
83+ </h2>
84+
85+ <div tal:attributes="id release/version/fmt:css-id/release-information-"
86+ tal:condition="python: release.release_notes or release.changelog">
87+
88+ <fieldset class="collapsible collapsed" style="padding: 0px;">
89+ <legend>Release information</legend>
90+
91+ <div tal:condition="release/release_notes">
92+ <strong>Release notes:</strong>
93+ <div style="margin-bottom: 0px;"
94+ tal:attributes="id release/version/fmt:css-id/release-notes-"
95+ tal:define="notes release/release_notes/fmt:shorten/800"
96+ tal:content="structure notes/fmt:text-to-html">
97+ ProductRelease.release_notes
98+ </div>
99+ </div>
100+
101+ <div tal:condition="release/changelog">
102+ <strong>Changelog:</strong>
103+ <div style="margin-bottom: 0px;"
104+ tal:attributes="id release/version/fmt:css-id/changelog-"
105+ tal:content="structure release/changelog/fmt:obfuscate-email/fmt:text-to-html">
106+ ProductRelease.changelog.
107+ </div>
108+ </div>
109+ </fieldset>
110+ </div>
111+ <div>
112+ <table class="listing" style="margin-top: 1em;">
113+ <thead>
114+ <tr>
115+ <th width="35%">File</th>
116+ <th width="45%">Description</th>
117+ <th>Downloads</th>
118+ <th tal:condition="has_edit">Delete</th>
119+ </tr>
120+ </thead>
121+ <tbody>
122+ <tr tal:repeat="file release/files">
123+ <tal:define-vars
124+ tal:define="checkbox_index string:${repeat/series/index}_${repeat/release/index}_${repeat/file/index}">
125+ <tal:release-file
126+ metal:use-macro="file/@@+macros/detailed_display" />
127 </tal:define-vars>
128- </tr>
129- </tbody>
130- <tfoot tal:condition="release/total_downloads">
131- <tr>
132- <th colspan="2" style="padding-top: 1em; text-align: right;">
133- Total downloads:
134- </th>
135- <td style="border: none; text-align: center;">
136- <span tal:replace="release/total_downloads/fmt:intcomma" />
137- </td>
138- </tr>
139- </tfoot>
140- </table>
141- </div>
142- <div tal:condition="not: release/total_downloads"
143- style="margin-top: 2em;">
144- </div>
145+ </tr>
146+ </tbody>
147+ <tfoot tal:condition="release/total_downloads">
148+ <tr>
149+ <th colspan="2" style="padding-top: 1em; text-align: right;">
150+ Total downloads:
151+ </th>
152+ <td style="border: none; text-align: center;">
153+ <span tal:replace="release/total_downloads/fmt:intcomma" />
154+ </td>
155+ <td style="border: none;"
156+ tal:condition="has_edit" />
157+ </tr>
158+ </tfoot>
159+ </table>
160+ </div>
161+ <div tal:condition="not: release/total_downloads"
162+ style="margin-top: 2em;" />
163
164- <div style="float:right"
165- tal:define="link release/menu:context/add_file"
166- tal:condition="link/enabled">
167- <a tal:replace="structure link/fmt:link" />
168- </div>
169- <div style="clear:both" />
170+ <tal:linkexists
171+ define="link release/menu:context/add_file"
172+ condition="link/enabled">
173+ <ul style="float: right; margin-top: 1em;">
174+ <li>
175+ <a tal:replace="structure link/fmt:link" />
176+ </li>
177+ </ul>
178+ </tal:linkexists>
179+ </div>
180 </tal:releasefilesexist>
181 </tal:releases>
182 <br />
183-
184 </tal:seriesfilesexist>
185
186- <p tal:condition="has_edit" class="add-files">
187- <tal:releases condition="series/releases">
188+ <tal:releases condition="series/releases">
189+ <p tal:condition="has_edit" class="add-files">
190 Add download file to the
191 <a tal:attributes="href series/fmt:url"
192 tal:content="series/name">name</a> series for release:
193 <tal:release repeat="release series/releases">
194 <a tal:attributes="href string:${series/name}/${release/version}/+adddownloadfile"
195 tal:content="release/version"
196- >version</a><tal:comma condition="not:repeat/release/end">,</tal:comma>
197+ >version</a><tal:comma condition="not:repeat/release/end">,</tal:comma>
198 </tal:release>
199- </tal:releases>
200- </p>
201+ </p>
202+ </tal:releases>
203
204 </div>
205 <input tal:condition="python: has_edit and view.has_download_files"