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
=== modified file 'lib/lp/registry/templates/product-files.pt'
--- lib/lp/registry/templates/product-files.pt 2009-10-22 15:58:34 +0000
+++ lib/lp/registry/templates/product-files.pt 2009-10-26 17:10:23 +0000
@@ -39,111 +39,114 @@
3939
40 <div40 <div
41 tal:repeat="series view/sorted_series_list"41 tal:repeat="series view/sorted_series_list"
42 tal:attributes="id string:series_${series/name}">42 tal:attributes="id series/name/fmt:css-id/series-">
43
44 <div tal:condition="not: repeat/series/start" class="portlet-border" />
4543
46 <tal:seriesfilesexist condition="series/has_release_files">44 <tal:seriesfilesexist condition="series/has_release_files">
45 <div tal:condition="not: repeat/series/start" class="portlet-border" />
47 <tal:releases repeat="release series/releases">46 <tal:releases repeat="release series/releases">
48 <tal:releasefilesexist condition="release/files">47 <tal:releasefilesexist condition="release/files">
49 <div tal:condition="not: repeat/release/start" class="portlet-border" />48 <div tal:condition="not: repeat/release/start" class="portlet-border" />
50 <div tal:condition="release/files" class="top-portlet">49 <div class="top-portlet">
51 <h2 style="font-size: 1.2em; float:left;">50 <h2 style="font-size: 1.2em;">
52 <a tal:attributes="href release/fmt:url">51 <a tal:attributes="href release/fmt:url">
53 <span tal:replace="release/name_with_codename" /> release</a>52 <span tal:replace="release/name_with_codename" /> release</a>
54 from the53 from the
55 <a tal:attributes="href series/fmt:url"54 <a tal:attributes="href series/fmt:url"
56 tal:content="series/name">name</a> series released55 tal:content="series/name">name</a> series released
57 <span56 <span
58 tal:attributes="title release/datereleased/fmt:datetime"57 tal:attributes="title release/datereleased/fmt:datetime"
59 tal:content="release/datereleased/fmt:approximatedate" />58 tal:content="release/datereleased/fmt:approximatedate" />
60 </h2>59 </h2>
6160
62 <div style="clear:both;" />61 <div tal:attributes="id release/version/fmt:css-id/release-information-"
6362 tal:condition="python: release.release_notes or release.changelog">
64 <div id="release_information"63
65 tal:condition="python: release.release_notes or release.changelog">64 <fieldset class="collapsible collapsed" style="padding: 0px;">
6665 <legend>Release information</legend>
67 <fieldset class="collapsible collapsed" style="padding: 0px;">66
68 <legend>Release information</legend>67 <div tal:condition="release/release_notes">
6968 <strong>Release notes:</strong>
70 <div tal:condition="release/release_notes">69 <div style="margin-bottom: 0px;"
71 <strong>Release notes:</strong>70 tal:attributes="id release/version/fmt:css-id/release-notes-"
72 <div id="release_notes" style="margin-bottom: 0px;"71 tal:define="notes release/release_notes/fmt:shorten/800"
73 tal:define="notes release/release_notes/fmt:shorten/800"72 tal:content="structure notes/fmt:text-to-html">
74 tal:content="structure notes/fmt:text-to-html">73 ProductRelease.release_notes
75 ProductRelease.release_notes74 </div>
76 </div>75 </div>
77 </div>76
7877 <div tal:condition="release/changelog">
79 <div tal:condition="release/changelog">78 <strong>Changelog:</strong>
80 <strong>Changelog:</strong>79 <div style="margin-bottom: 0px;"
81 <div id="changelog" style="margin-bottom: 0px;"80 tal:attributes="id release/version/fmt:css-id/changelog-"
82 tal:content="structure release/changelog/fmt:obfuscate-email/fmt:text-to-html">81 tal:content="structure release/changelog/fmt:obfuscate-email/fmt:text-to-html">
83 ProductRelease.changelog.82 ProductRelease.changelog.
84 </div>83 </div>
85 </div>84 </div>
86 </fieldset>85 </fieldset>
87 </div> <!-- release information -->86 </div>
8887 <div>
89 <table class="listing" style="margin-top: 1em;">88 <table class="listing" style="margin-top: 1em;">
90 <thead>89 <thead>
91 <tr>90 <tr>
92 <th width="35%">File</th>91 <th width="35%">File</th>
93 <th width="45%">Description</th>92 <th width="45%">Description</th>
94 <th>Downloads</th>93 <th>Downloads</th>
95 <th tal:condition="has_edit">Delete</th>94 <th tal:condition="has_edit">Delete</th>
96 </tr>95 </tr>
97 </thead>96 </thead>
98 <tbody>97 <tbody>
99 <tr tal:repeat="file release/files">98 <tr tal:repeat="file release/files">
100 <tal:define-vars99 <tal:define-vars
101 tal:define="checkbox_index string:${repeat/series/index}_${repeat/release/index}_${repeat/file/index}">100 tal:define="checkbox_index string:${repeat/series/index}_${repeat/release/index}_${repeat/file/index}">
102 <tal:release-file101 <tal:release-file
103 metal:use-macro="file/@@+macros/detailed_display" />102 metal:use-macro="file/@@+macros/detailed_display" />
104 </tal:define-vars>103 </tal:define-vars>
105 </tr>104 </tr>
106 </tbody>105 </tbody>
107 <tfoot tal:condition="release/total_downloads">106 <tfoot tal:condition="release/total_downloads">
108 <tr>107 <tr>
109 <th colspan="2" style="padding-top: 1em; text-align: right;">108 <th colspan="2" style="padding-top: 1em; text-align: right;">
110 Total downloads:109 Total downloads:
111 </th>110 </th>
112 <td style="border: none; text-align: center;">111 <td style="border: none; text-align: center;">
113 <span tal:replace="release/total_downloads/fmt:intcomma" />112 <span tal:replace="release/total_downloads/fmt:intcomma" />
114 </td>113 </td>
115 </tr>114 <td style="border: none;"
116 </tfoot>115 tal:condition="has_edit" />
117 </table>116 </tr>
118 </div>117 </tfoot>
119 <div tal:condition="not: release/total_downloads"118 </table>
120 style="margin-top: 2em;">119 </div>
121 </div>120 <div tal:condition="not: release/total_downloads"
121 style="margin-top: 2em;" />
122122
123 <div style="float:right"123 <tal:linkexists
124 tal:define="link release/menu:context/add_file"124 define="link release/menu:context/add_file"
125 tal:condition="link/enabled">125 condition="link/enabled">
126 <a tal:replace="structure link/fmt:link" />126 <ul style="float: right; margin-top: 1em;">
127 </div>127 <li>
128 <div style="clear:both" />128 <a tal:replace="structure link/fmt:link" />
129 </li>
130 </ul>
131 </tal:linkexists>
132 </div>
129 </tal:releasefilesexist>133 </tal:releasefilesexist>
130 </tal:releases>134 </tal:releases>
131 <br />135 <br />
132
133 </tal:seriesfilesexist>136 </tal:seriesfilesexist>
134137
135 <p tal:condition="has_edit" class="add-files">138 <tal:releases condition="series/releases">
136 <tal:releases condition="series/releases">139 <p tal:condition="has_edit" class="add-files">
137 Add download file to the140 Add download file to the
138 <a tal:attributes="href series/fmt:url"141 <a tal:attributes="href series/fmt:url"
139 tal:content="series/name">name</a> series for release:142 tal:content="series/name">name</a> series for release:
140 <tal:release repeat="release series/releases">143 <tal:release repeat="release series/releases">
141 <a tal:attributes="href string:${series/name}/${release/version}/+adddownloadfile"144 <a tal:attributes="href string:${series/name}/${release/version}/+adddownloadfile"
142 tal:content="release/version"145 tal:content="release/version"
143 >version</a><tal:comma condition="not:repeat/release/end">,</tal:comma>146 >version</a><tal:comma condition="not:repeat/release/end">,</tal:comma>
144 </tal:release>147 </tal:release>
145 </tal:releases>148 </p>
146 </p>149 </tal:releases>
147150
148 </div>151 </div>
149 <input tal:condition="python: has_edit and view.has_download_files"152 <input tal:condition="python: has_edit and view.has_download_files"