Code review comment for lp:~sinzui/launchpad/porlet-improvements

On Fri, 2009-09-04 at 09:36 +0000, Abel Deuring wrote:
> Review: Approve
> Hi Curtis,
>
> nice branch. I have only a few minor remarks.
>
>
> > === modified file 'lib/canonical/launchpad/doc/tales.txt'
> > --- lib/canonical/launchpad/doc/tales.txt 2009-08-20 12:24:29 +0000
> > +++ lib/canonical/launchpad/doc/tales.txt 2009-09-03 17:59:56 +0000
> > @@ -214,6 +214,19 @@
> > >>> test_tales('foo/fmt:shorten/8', foo='abcdefghij')
> > 'abcde...'
> >
> > +To ellisize the middle of a string. use fmt:ellipsize and pass the max
>
> s/ellisize/ellipsize/

Fixed.

> > === modified file 'lib/canonical/launchpad/icing/style-3-0.css'
> > --- lib/canonical/launchpad/icing/style-3-0.css 2009-09-02 14:19:45 +0000
> > +++ lib/canonical/launchpad/icing/style-3-0.css 2009-09-03 21:30:01 +0000
> [...]
> > @@ -564,17 +564,20 @@
> > border-radius: 3px;
> > background: #4f843c url(/@@/bg-project-downloads.png) center right no-repeat;
> > padding: 6%;
> > - padding-right: 50px;
> > + padding-right: 40px;
> > color: #fff;
> > - font-size: 108%;
> > - text-decoration: underline;
> > + font-size: 97%;
>
> This is for ".downloads li a", i.e., for a "smaller" font size, not
> for a heading. If we assume that the base font size is 20px (it is
> is most real-world cases smaller, I think), it will be reduced to
> 19.4px -- that's hardly noticable. Couldn't the size stay at 100%?

The table of sizes is in style-3.0.css. 100% is 13px. The body is set to
93% (12px). You have caught a typo. I intended to type 93% to make the
font the same as the rest of the content.

> > === modified file 'lib/canonical/launchpad/webapp/tales.py'
> > --- lib/canonical/launchpad/webapp/tales.py 2009-08-31 12:01:46 +0000
> > +++ lib/canonical/launchpad/webapp/tales.py 2009-09-03 21:23:25 +0000
>
> [...]
>
> > @@ -2830,6 +2831,16 @@
> > else:
> > return self._stringtoformat
> >
> > + def ellipsize(self, maxlength):
> > + """Use like tal:content="context/foo/fmt:ellipsize/60"."""
> > + if len(self._stringtoformat) > maxlength:
> > + length = (maxlength - 3) / 2
> > + return (
> > + self._stringtoformat[:length] + '...' +
> > + self._stringtoformat[-length:])
>
> Not a real issue, but if maxlength is an even number, this return
> a string with maxlength - 1 characters. What about using
>
> self._stringtoformat[:maxlength - length - 3] + ...

Your approach is better, I used it.