Code review comment for lp:~bac/launchpad/bug-461164-downloadfix

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)

« Back to merge proposal