loggerhead

Merge lp://staging/~pr0gg3d/loggerhead/long_comments_wrap_276768 into lp://staging/loggerhead

long_comments_wrap_276768
Merge into trunk-rich

Proposed by Francesco Del Degan on 2012-01-21

Status:

Merged

Approved by:

j.c.sackett on 2012-01-26

Approved revision:

463

Merged at revision:

474

Proposed branch:

lp://staging/~pr0gg3d/loggerhead/long_comments_wrap_276768

Merge into:

lp://staging/loggerhead

Diff against target:

11 lines (+1/-0)

1 file modified

loggerhead/static/css/diff.css (+1/-0)

To merge this branch:

bzr merge lp://staging/~pr0gg3d/loggerhead/long_comments_wrap_276768

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
j.c.sackett (community)		2012-01-26	Approve on 2012-01-26
Richard Harding (community)	code*	2012-01-21	Approve on 2012-01-26
Review via email: mp+89521@code.staging.launchpad.net

Description of the change

As referred to #276768:

That comment string is rendered using (revisioninfo.pt):

that explicitly "expand tabs and turn spaces into "non-breaking spaces", so browsers won't chop up the string.", so that "white-space" CSS controls won't work.

We could use CSS3 "word-wrap: break-word" (diff.css) or avoid the use of util.fixed_width().

This branch uses former solution.

Revision history for this message

Martin Pool (mbp) wrote on 2012-01-23:

Thanks, Francesco.

If we're using the word-wrap css property, do we still need/want
escaping to nbsp? I would have thought not...
--
Martin

Revision history for this message

Francesco Del Degan (pr0gg3d) wrote on 2012-01-24:

If we'll get rid of (that is the latter solution that i've proposed) util.fixed_width() we don't need the word-wrap css property at all, because "wrapping" is the default behaviour (using default white-space css property). I could make a branch with this solution.

I'm trying to figure out why fixed_width was used there.

Revision history for this message

Richard Harding (rharding) wrote on 2012-01-26:

Personally I'd rather see the solution that wasn't css3 specific to be more cross browser compatible, but looks like this should work.

review: Approve (code*)

Revision history for this message

j.c.sackett (jcsackett) wrote on 2012-01-26:

I see no problem with using the css3 property, as we do on launchpad and LP is loggerhead's primary user.

This looks fine.

review: Approve