-
-
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js (Diff revision 1) There's no good reason to put the
.html
on the next line. It just causes more length and indentation.
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: |
|
|||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+111 -4) |
-
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: |
|
---|
-
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