Keep consistent column widths when wrapping long lines in diffs.

Review Request #6013 — Created June 20, 2014 and submitted

Information

Review Board
release-2.0.x
34a5a67...

Reviewers

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.

daviddavid
david
  1. I'd like to see your testing include added and deleted files, and possibly even error states.

  2. Show all issues

    There's no good reason to put the .html on the next line. It just causes more length and indentation.

    1. Had this from before when I was appending the whole thing at this location.

  3. 
      
chipx86
david
  1. 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"?

  2. 
      
chipx86
david
  1. Ship It!

  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (49c378b)
reviewbot
  1. 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
    
    
  2. 
      
Loading...