loggerhead

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

  1. long_comments_wrap_276768
  2. Merge into trunk-rich
Proposed by Francesco Del Degan
Status: Merged
Approved by: j.c.sackett
Approved revision: 463
Merged at revision: 474
Proposed branch: lp:~pr0gg3d/loggerhead/long_comments_wrap_276768
Merge into: lp: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:~pr0gg3d/loggerhead/long_comments_wrap_276768
Reviewer Review Type Date Requested Status
j.c.sackett (community) Approve
Richard Harding (community) code* Approve
Review via email: mp+89521@code.launchpad.net

Description of the change

As referred to #276768:

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

<div class="information" tal:content="structure python:util.fixed_width(change.comment)"></div>

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.

To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

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 :

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 :

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 :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'loggerhead/static/css/diff.css'
2--- loggerhead/static/css/diff.css 2011-02-18 05:41:17 +0000
3+++ loggerhead/static/css/diff.css 2012-01-21 08:45:27 +0000
4@@ -29,6 +29,7 @@
5 margin: 0 0 5px 0;
6 font-size: 77%;
7 color: #666;
8+ word-wrap: break-word;
9 }
10 /*Diff Boxes*/
11 .diffBox {

Subscribers

People subscribed via source and target branches