• 
      

    Refactor CSS for DiffCommit listings

    Review Request #7220 — Created April 17, 2015 and submitted

    Information

    Review Board
    dvcs
    9749719...

    Reviewers

    All the CSS for styling tables of DiffCommits has been refactored
    into diffviewer.less to reduce duplication. The styling can now be
    more easily made consistent across all views where it is shown.

    Viewed a review request with a change description and the diff
    viewer of a review request with multiple commits.

    Description From Last Updated

    This is copied from the .diff-index code below here. How should this be refactored?

    brenniebrennie

    No blank line.

    chipx86chipx86

    While this isn't a strict rule, a good starting point is to alphabetize the rules. There are exceptions. For instance, …

    chipx86chipx86

    Can you add a constant for this color?

    chipx86chipx86

    Alphabetical order.

    chipx86chipx86

    We probably want the author to shrink to fit, and the summary to expand to take all remaining space. For …

    chipx86chipx86

    This td should go before the tr above (alphabetical). That, or it should nest.

    chipx86chipx86

    These should be swapped. Children go after the immediate selector's rules.

    chipx86chipx86
    brennie
    reviewbot
    1. Tool: PEP8 Style Checker
      Ignored Files:
          reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
          reviewboard/templates/reviews/boxes/commit_list.html
          reviewboard/templates/reviews/boxes/commit_list_change.html
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/static/rb/css/pages/diffviewer.less
      
      
      
      Tool: Pyflakes
      Ignored Files:
          reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
          reviewboard/templates/reviews/boxes/commit_list.html
          reviewboard/templates/reviews/boxes/commit_list_change.html
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/static/rb/css/pages/diffviewer.less
      
      
    2. 
        
    brennie
    1. 
        
    2. reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 1)
       
       
       
       
       
       
       
       
      Show all issues

      This is copied from the .diff-index code below here. How should this be refactored?

      1. If possible, I'd just make .diff-file-icon it's own top-most rule. That way, it only needs to be styled once.

        We don't need (or sometimes even want) to deeply nest all rules. We do it to prevent collisions a lot of the time, but in this case, we have a common style between two parts of the UI, so it can be promoted.

    3. 
        
    brennie
    reviewbot
    1. Tool: Pyflakes
      Ignored Files:
          reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
          reviewboard/templates/reviews/boxes/commit_list.html
          reviewboard/templates/reviews/boxes/commit_list_change.html
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/static/rb/css/pages/diffviewer.less
      
      
      
      Tool: PEP8 Style Checker
      Ignored Files:
          reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
          reviewboard/templates/reviews/boxes/commit_list.html
          reviewboard/templates/reviews/boxes/commit_list_change.html
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/static/rb/css/pages/diffviewer.less
      
      
    2. 
        
    chipx86
    1. 
        
    2. reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 2)
       
       
       
       
      Show all issues

      No blank line.

    3. reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 2)
       
       
       
       
      Show all issues

      While this isn't a strict rule, a good starting point is to alphabetize the rules. There are exceptions. For instance, position often goes with top, left, etc., and width and height often go together. In a simple case like this, though, it's preferrable to alphabetize.

    4. Show all issues

      Can you add a constant for this color?

    5. Show all issues

      Alphabetical order.

    6. reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 2)
       
       
       
       
       
       
       
       
      Show all issues

      We probably want the author to shrink to fit, and the summary to expand to take all remaining space. For this, I'd suggest something like:

      &.diff-commit-author {
        white-space: nowrap;
      }
      
      &.diff-commit-summary {
        width: 100%;
      }
      

      (Also, notice the alphabetical order.)

    7. Show all issues

      This td should go before the tr above (alphabetical). That, or it should nest.

      1. The td rule is nested in the tr rule.

    8. reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 2)
       
       
       
       
       
       
      Show all issues

      These should be swapped. Children go after the immediate selector's rules.

    9. 
        
    brennie
    reviewbot
    1. Tool: Pyflakes
      Ignored Files:
          reviewboard/static/rb/css/pages/diffviewer.less
          reviewboard/templates/reviews/boxes/commit_list.html
          reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/static/rb/css/defs.less
          reviewboard/templates/reviews/boxes/commit_list_change.html
      
      
      
      Tool: PEP8 Style Checker
      Ignored Files:
          reviewboard/static/rb/css/pages/diffviewer.less
          reviewboard/templates/reviews/boxes/commit_list.html
          reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/static/rb/css/defs.less
          reviewboard/templates/reviews/boxes/commit_list_change.html
      
      
    2. 
        
    david
    1. Ship It!
    2. 
        
    brennie
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to dvcs (6222bd3)