Merge lp:~thumper/launchpad/code-review-comment-formatting into lp:launchpad

Proposed by Tim Penhey
Status: Merged
Merged at revision: 8983
Proposed branch: lp:~thumper/launchpad/code-review-comment-formatting
Merge into: lp:launchpad
Diff against target: None lines
To merge this branch: bzr merge lp:~thumper/launchpad/code-review-comment-formatting
Reviewer Review Type Date Requested Status
Martin Albisetti (community) ui Approve
Abel Deuring (community) Approve
Canonical Launchpad Engineering browser-qa Pending
Review via email: mp+9176@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

= Summary =

Fixes bug 326129 (firefox-3.{5,6} bug, merge review comments don't get wrapped
in <pre> block).

== Proposed fix ==

Adding the "white-space: pre-wrap" to the nice_pre formatted <pre> tag.

== Implementation details ==

Uses the "pre.wrap" CSS style now, rather than inline styles.

I was reminded of why we don't use the bug formatter for code review comments.
The bug formatter is turning an email into HTML, by grouping lines into
paragraphs. This works really well for actual conversations, but is complete
shite for code.

A question for beuno:
  should the pre.wrap class be added to style-3-0.css?

== Demo and Q/A ==

Look at a code review comment :)

== Reviewers ==

Need a general review:
  reviewer launchpad
And a UI review
  reviewer beuno ui
And a QA review to actually test against FF3.5, Konq, Opera and IE
  reviewer launchpad browser-qa

Revision history for this message
Abel Deuring (adeuring) :
review: Approve
Revision history for this message
Martin Albisetti (beuno) :
review: Approve (ui)
Revision history for this message
Martin Albisetti (beuno) wrote :

Yes, I think pre.wrap should be in the 3.0 CSS

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/launchpad/icing/style.css'
--- lib/canonical/launchpad/icing/style.css 2009-07-16 04:17:24 +0000
+++ lib/canonical/launchpad/icing/style.css 2009-07-23 04:21:36 +0000
@@ -2418,6 +2418,7 @@
2418pre.wrap {2418pre.wrap {
2419 white-space: -moz-pre-wrap;2419 white-space: -moz-pre-wrap;
2420 white-space: -o-pre-wrap;2420 white-space: -o-pre-wrap;
2421 white-space: pre-wrap;
2421 word-wrap: break-word;2422 word-wrap: break-word;
2422}2423}
24232424
24242425
=== modified file 'lib/canonical/launchpad/webapp/tales.py'
--- lib/canonical/launchpad/webapp/tales.py 2009-07-17 00:26:05 +0000
+++ lib/canonical/launchpad/webapp/tales.py 2009-07-23 04:21:36 +0000
@@ -2469,8 +2469,6 @@
2469 fall back for IE by using the IE specific word-wrap property.2469 fall back for IE by using the IE specific word-wrap property.
24702470
2471 TODO: Test IE compatibility. StuartBishop 200411182471 TODO: Test IE compatibility. StuartBishop 20041118
2472 TODO: This should probably just live in the stylesheet if this
2473 CSS implementation is good enough. StuartBishop 20041118
2474 """2472 """
2475 if not self._stringtoformat:2473 if not self._stringtoformat:
2476 return self._stringtoformat2474 return self._stringtoformat
@@ -2478,13 +2476,7 @@
2478 linkified_text = re_substitute(self._re_linkify,2476 linkified_text = re_substitute(self._re_linkify,
2479 self._linkify_substitution, break_long_words,2477 self._linkify_substitution, break_long_words,
2480 cgi.escape(self._stringtoformat))2478 cgi.escape(self._stringtoformat))
2481 return ('<pre style="'2479 return '<pre class="wrap">%s</pre>' % linkified_text
2482 'white-space: -moz-pre-wrap;'
2483 'white-space: -o-pre-wrap;'
2484 'word-wrap: break-word;'
2485 '">%s</pre>'
2486 % linkified_text
2487 )
24882480
2489 # Match lines that start with one or more quote symbols followed2481 # Match lines that start with one or more quote symbols followed
2490 # by a space. Quote symbols are commonly '|', or '>'; they are2482 # by a space. Quote symbols are commonly '|', or '>'; they are