Launchpad itself

Code review comment for lp:~sinzui/launchpad/css-ui-0

  1. css-ui-0
  2. Merge into devel
Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

Hi Curtis,

This branch looks good. I just have some minor comments.

-Edwin

>=== modified file 'lib/canonical/launchpad/offline-maintenance-haproxy.html'
>--- lib/canonical/launchpad/offline-maintenance-haproxy.html 2010-04-01 13:43:01 +0000
>+++ lib/canonical/launchpad/offline-maintenance-haproxy.html 2010-12-16 18:25:39 +0000
>@@ -6,7 +6,17 @@
> <head>
> <title>Launchpad is offline for maintenance</title>
> <style type="text/css" media="screen, print">
>- .offline{text-align:center;margin:2em 0}
>+ html, body {
>+ font-family: UbuntuBeta, Ubuntu, 'Bitstream Vera Sans', 'DejaVu Sans', Tahoma, sans-serif;
>+ }
>+ .offline {
>+ text-align: center;
>+ max-width: 60em;
>+ }
>+ .offline > * {
>+ margin-left: auto;
>+ margin-right: auto;
>+ }
> </style>
> </head>
> <body>

This file has CR/LF. How did that happen?

>=== modified file 'lib/canonical/launchpad/offline-unplanned-haproxy.html'
>--- lib/canonical/launchpad/offline-unplanned-haproxy.html 2010-03-02 10:42:06 +0000
>+++ lib/canonical/launchpad/offline-unplanned-haproxy.html 2010-12-16 18:25:39 +0000
>@@ -6,7 +6,17 @@
> <head>
> <title>Please try again</title>
> <style type="text/css" media="screen, print">
>- .offline{text-align:center;margin:2em 0}
>+ html, body {
>+ font-family: UbuntuBeta, Ubuntu, 'Bitstream Vera Sans', 'DejaVu Sans', Tahoma, sans-serif;
>+ }
>+ .offline {
>+ text-align: center;
>+ max-width: 60em;
>+ }
>+ .offline > * {
>+ margin-left: auto;
>+ margin-right: auto;
>+ }
> </style>
> </head>
> <body>

CR/LF again.

>=== modified file 'lib/canonical/launchpad/webapp/interfaces.py'
>--- lib/canonical/launchpad/webapp/interfaces.py 2010-11-02 19:55:24 +0000
>+++ lib/canonical/launchpad/webapp/interfaces.py 2010-12-16 18:25:39 +0000
>@@ -568,15 +568,12 @@
> """Matches the standard logging levels, with the addition of notice
> (which we should probably add to our log levels as well)
> """

The above docstring should be updated.

>- # XXX mpt 2006-03-22 bugs=36287:
>- # NOTICE and INFO should be merged.
> DEBUG = logging.DEBUG # A debugging message
> INFO = logging.INFO # simple confirmation of a change
>- NOTICE = logging.INFO + 5 # action had effects you might not have intended
> WARNING = logging.WARNING # action will not be successful unless you ...
> ERROR = logging.ERROR # the previous action did not succeed, and why
>
>- ALL_LEVELS = (DEBUG, INFO, NOTICE, WARNING, ERROR)
>+ ALL_LEVELS = (DEBUG, INFO, WARNING, ERROR)
>
>
> class INotification(Interface):
>=== modified file 'lib/canonical/launchpad/webapp/notifications.py'
>--- lib/canonical/launchpad/webapp/notifications.py 2010-08-20 20:31:18 +0000
>+++ lib/canonical/launchpad/webapp/notifications.py 2010-12-16 18:25:39 +0000
>@@ -334,8 +327,6 @@
> structured('Debug notification <b>%d</b>' % count))
> response.addInfoNotification(
> structured('Info notification <b>%d</b>' % count))
>- response.addNoticeNotification(
>- structured('Notice notification <b>%d</b>' % count))
> response.addWarningNotification(
> structured('Warning notification <b>%d</b>' %count))

Can you add a space between "%count"?

review: Approve (code)

« Back to merge proposal