Code review comment for lp:~edwin-grubbs/launchpad/bug-234051-bug-project-info
Revision history for this message
| Edwin Grubbs (edwin-grubbs) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
a60fb26
(Get the code!)
Hi Barry,
Thanks for the review. In addition to your comments that I addressed
below, I added the padding that Martin asked for and the change in
badge alignment that Curtis asked for.
On Fri, Nov 6, 2009 at 12:45 PM, Barry Warsaw <email address hidden> wrote:
> Review: Approve code ui*
>> In the "Assigned bugs" portlet (person-
>> a table to display the badges like other portlets do. I also added an
>> additional line that lists all the pillars that a given bug affects.
>
> This looks great. The one question I have is why all the pillars are not
> linked? Looking at the screenshot I see that e.g. for bug #9 Ubuntu is
> linked, but not alsa-utils, Evolution, or Mozilla Thunderbird. How hard would
> it be to link all of those?
Not hard. I'll do that.
>> I also removed the $distribution/
>> 196322.
>>
>> I removed the italics from the "registered" css class as requested
>> in bug 465524 to fix the "Latest memberships" portlet.
>
> Yay!
>
> === modified file 'lib/canonical/
> --- lib/canonical/
> +++ lib/canonical/
>> @@ -424,10 +424,6 @@
>> .registered {
>> font-size: 85%;
>> color: #666;
>> - font-style: italic;
>> - }
>> -li .registered {
>> - font-style: normal;
>
> There's a subtle change here. Not only are you deleting the italics style
> from .registered (good) but you're also adding the font-size and color styles
> to li.registered. That might be okay, but is it what you intended?
The "li .registered" selector doesn't eliminate all the styles from
the ".registered", just the ones that are explicitely overridden
(font-style), so elements with the "registered" class inside an <li>
were already getting their font-size and color set as it is now.
>> }
>> .description {
>> clear: both;
>
> === renamed file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
>> @@ -1,28 +1,38 @@
>> -<tr xmlns:tal="http://
>> - <td class="icon left" tal:content=
>> - <td>
>> - <div>
>> - #<span tal:replace=
>> - <a tal:attributes=
>> - tal:content=
>> - </a>
>> - <tal:badges replace="structure context/
>> - </div>
>> - <div class="lesser" tal:condition=
>> - <tal:affected_
>> - condition=
>> - also in
>> - <tal:per_pillar repeat="pillar also_in">
>> - <b tal:content=
>> - </tal:per_pillar>
>> - </tal:affected_
>> - reported
>> - <span
>> - tal:attributes=
>> - tal:content=
>> - by
>> - <a tal:attributes=
>> - tal:content=
>> - </div>
>> - </td>
>> -</tr>
>> +<metal:header
>> + xmlns:metal="http://
>> + xmlns:tal="http://
>> + define-
>> + <tal:comment replace="nothing">
>> + Variables to be defined for this macro.
>> + :bugtask: (Required) BugTask that will be displayed.
>> + :show_extra_
>> + to display a second line of info below the bug title.
>
> Very nice. Thanks for adding this comment.
>
>> + </tal:comment>
>> + <tr>
>> + <td class="icon left" tal:content=
>> + <td>
>> + #<span tal:replace=
>> + <a tal:attributes=
>> + tal:content=
>> + </a>
>> + </td>
>> + <td>
>> + <tal:badges replace="structure bugtask/
>> + </td>
>> + </tr>
>> + <tr tal:condition=
>> + <td colspan="3">
>> + <div class="registered">
>> + <tal:affected_
>> + in <a tal:attributes=
>> + tal:content=
>> + /><tal:comma condition=
>> + <tal:per_pillar repeat="pillar also_in">
>> + <tal:pillar replace=
>> + /><tal:comma condition="not: repeat/
>> + </tal:per_pillar>
>> + </tal:affected_
>> + </div>
>> + </td>
>> + </tr>
>> +</metal:header>
>
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
>> @@ -296,7 +296,7 @@
>>
>> usedfor = IDistribution
>> facet = 'overview'
>> - links = ['edit', 'branding', 'driver', 'search', 'allpkgs', 'members',
>> + links = ['edit', 'branding', 'driver', 'search', 'members',
>> 'mirror_admin', 'reassign', 'addseries', 'series', 'milestones',
>> 'top_contributors',
>> 'builds', 'cdimage_mirrors', 'archive_mirrors',
>> @@ -360,10 +360,6 @@
>> enabled = self._userCanSe
>> return Link('+
>>
>> - def allpkgs(self):
>> - text = 'List all packages'
>> - return Link('+
>> -
>> @enabled_
>> def members(self):
>> text = 'Change members team'
>> @@ -717,16 +713,6 @@
>> distribution=
>>
>>
>> -class DistributionAll
>
> Don't forget to remove this from the module's __all__.
Fixed.
>> - """A view to show all the packages in a distribution."""
>> -
>> - def initialize(self):
>> - results = self.context.
>> - self.batchnav = BatchNavigator(
>> -
>> - label = 'All packages'
>> -
>> -
>> class DistributionSet
>> """Action menu for `DistributionSe
>
>
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
>> @@ -20,8 +20,8 @@
>>
>> <table>
>> <tr tal:repeat="bugtask bugtasks">
>> - <tal:define-vars define=
>> - <tal:task tal:replace=
>> + <tal:define-vars define=
>> + <metal:task use-macro=
>> </tal:define-vars>
>> </tr>
>> </table>
>
> merge-conditional, r=me with consideration of above
> ui* with consideration of linking the other pillars
> --
> https:/
> You are the owner of lp:~edwin-grubbs/launchpad/bug-234051-bug-project-info.
>
Here is the incremental diff.
{{{
=== modified file 'lib/lp/
--- lib/lp/
+++ lib/lp/
@@ -16,20 +16,21 @@
</a>
</td>
- <td>
+ <td style="text-align: right">
<tal:badges replace="structure bugtask/
</td>
</tr>
<tr tal:condition=
- <td colspan="3">
+ <td colspan="3" style="
<div class="registered">
in <a tal:attributes=
- <tal:pillar replace=
- /><tal:comma condition="not: repeat/
+ <a tal:attributes=
+ tal:content=
+ /><tal:comma condition="not: repeat/
</div>
=== modified file 'lib/lp/
--- lib/lp/
+++ lib/lp/
@@ -8,7 +8,6 @@
__all__ = [
'Derivativ
'Distribut
- 'DistributionAl
'Distribut
'Distribut
'Distribut
}}}