Keep consistent column widths when wrapping long lines in diffs.
Review Request #6013 — Created June 20, 2014 and submitted
We've historically had issues with very long, unbreakable lines. Prior
to 2.0, they would result in a horizontal scrollbar on the page,
requiring a lot of scrolling. In 2.0, they simply disappeared off the
edge of the parent container, with no ability to see the rest of the
contents.In a best-case scenario, with long breakable lines, one column would end
up growing really wide while the other shrunk considerably (often
resulting in many rows taken for a single line, growing the page
length).This change adds some sane management to the column widths. In
combination with the existing'word-wrap: break-word;', we now set
minimum and maximum widths for the columns every time the page's width
changes. These widths are calculated based on the container's width,
factoring in the number of content columns and the widths taken by the
line numbers.Each column gets a maximum of half and a minimum of 1/3 of the available
area. That maximum width, along with the word-wrap setting, allows long
non-breaking strings to wrap sanely, and also ensures that all wrapping
is constrained within a reasonable area to prevent the other column from
shrinking beyond readability.
Tested with normal diffs and ones with really long lines of random,
unbreakable characters.I was able to resize the browser width to any width, and still get
readable, wrapped diffs on both sides, in both Chrome and Firefox.Tested with modified files and added files, both with very long lines.
Tested with deleted files.
Generated an error with a diff and saw that the error box looked correct.
| Description | From | Last Updated |
|---|---|---|
|
There's no good reason to put the .html on the next line. It just causes more length and indentation. |
 david
|
- Change Summary:
-
- Fixed giving new files a full width (I changed a variable from a 'hasTwoColumns' bool to a column count before initially putting up for review, and was checking it incorrectly).
- Fixed the error traceback display being shortend to half the width of the box.
- Added additional padding to give some room for the text on the right.
- Cleaned up a call to
.html().
- Testing Done:
-
Tested with normal diffs and ones with really long lines of random,
unbreakable characters. I was able to resize the browser width to any width, and still get
readable, wrapped diffs on both sides, in both Chrome and Firefox. + + Tested with modified files and added files, both with very long lines.
+ + Tested with deleted files.
+ + Generated an error with a diff and saw that the error box looked correct.
- Commit:
-
c3a8562ac95eb409cf4bea50e64eec96b2dc7a3134a5a67a2cb815dd435186fcf5646a967c9f07c9
-
The code looks good, but part of your description is very confusing: "Each column gets a maximum width of half that available area, and a minimum width of 2/3 of that area."
Maybe say "a maximum of half and a minimum of 1/3 of the available area"?
- Description:
-
We've historically had issues with very long, unbreakable lines. Prior
to 2.0, they would result in a horizontal scrollbar on the page, requiring a lot of scrolling. In 2.0, they simply disappeared off the edge of the parent container, with no ability to see the rest of the contents. In a best-case scenario, with long breakable lines, one column would end
up growing really wide while the other shrunk considerably (often resulting in many rows taken for a single line, growing the page length). This change adds some sane management to the column widths. In
combination with the existing 'word-wrap: break-word;', we now setminimum and maximum widths for the columns every time the page's width changes. These widths are calculated based on the container's width, factoring in the number of content columns and the widths taken by the line numbers. ~ Each column gets a maximum width of half that available area, and a
~ minimum width of 2/3 of that area. That maximum width, along with the ~ word-wrap setting, allows long non-breaking strings to wrap sanely, and ~ also ensures that all wrapping is constrained within a reasonable area ~ to prevent the other column from shrinking beyond readability. ~ Each column gets a maximum of half and a minimum of 1/3 of the available
~ area. That maximum width, along with the word-wrap setting, allows long ~ non-breaking strings to wrap sanely, and also ensures that all wrapping ~ is constrained within a reasonable area to prevent the other column from ~ shrinking beyond readability.
-
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/diffviewer/views/chunkHighlighterView.js Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/diffviewer/views/chunkHighlighterView.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/diffviewer/views/chunkHighlighterView.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/js/diffviewer/views/chunkHighlighterView.js