Add previous and next buttons to navigate between file attachments

Review Request #8443 - Created Sept. 27, 2016 and submitted

Barret Rennie
Review Board
release-3.0.x
0992759...
reviewboard

The file attachment UI previously did not have any way to navigate
between any file attachments. A user would have to navigate back to the
review request and select the next one they wished to review.

Now there are previous and next buttons on the edges of the page when
there are more file attachments to review. They follow the order of the
review request UI and only navigate to active (and latest revisions).
When they are hovered over, they expand out to show the thumbnail of the
attachment that will be navigated to.

Ran unit tests.

Tested in the following browsers on OSX:

  • Chrome
  • Firefox release
  • Firefox developer edition
  • Safari

Verified the links were correct on local site review requests.

Loading file attachments...

  • 0
  • 0
  • 21
  • 8
  • 29
Description From Last Updated
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
David Trowbridge
  1. 
      
  2. reviewboard/reviews/ui/base.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    It might be nice to break this out into a method, or at least comment it and explain what it's doing.

  3. reviewboard/reviews/ui/base.py (Diff revision 1)
     
     

    Perhaps we can change get_latest_file_attachments to return a generator expression instead of a list?

  4. reviewboard/reviews/views.py (Diff revision 1)
     
     

    I don't think this change is correct.

    1. Thanks for catching that.

  5. Where does the 14px come from?

  6. There's an extra space before this templatetag.

  7. reviewboard/templates/reviews/ui/base.html (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    This is all indented 1 space too much.

  8. reviewboard/templates/reviews/ui/base.html (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Indented 1 space too much.

  9. 
      
Barret Rennie
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. reviewboard/reviews/ui/base.py (Diff revision 2)
     
     
     undefined name 'next_obj'
    
  3. reviewboard/reviews/ui/base.py (Diff revision 2)
     
     
     undefined name 'prev_obj'
    
  4. reviewboard/reviews/ui/base.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (117 > 79 characters)
    
  5. 
      
Barret Rennie
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. reviewboard/reviews/ui/base.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (117 > 79 characters)
    
  3. 
      
David Trowbridge
  1. 
      
  2. Can we give this a bit of a box-shadow so that the layering is more apparent?

  3. reviewboard/reviews/ui/base.py (Diff revision 3)
     
     

    It might be nice to say what "next" and "previous" mean.

  4. 
      
Barret Rennie
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. reviewboard/reviews/ui/base.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (117 > 79 characters)
    
  3. 
      
David Trowbridge
  1. 
      
  2. reviewboard/static/rb/css/pages/reviews.less (Diff revisions 3 - 4)
     
     

    Can we define a constant for this?

    1. Replaced this with a calc() expression.

  3. 
      
Barret Rennie
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. reviewboard/reviews/ui/base.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (117 > 79 characters)
    
  3. 
      
David Trowbridge
  1. 
      
  2. reviewboard/static/rb/css/pages/reviews.less (Diff revisions 4 - 5)
     
     

    If we made a change to have @page-container-padding be in px instead of em, we could use LESS's calculations here instead of calc().

  3. 
      
Barret Rennie
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. reviewboard/reviews/ui/base.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (117 > 79 characters)
    
  3. 
      
Barret Rennie
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. reviewboard/reviews/ui/base.py (Diff revision 7)
     
     
    Col: 80
     E501 line too long (117 > 79 characters)
    
  3. 
      
Barret Rennie
Barret Rennie
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. reviewboard/reviews/ui/base.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (117 > 79 characters)
    
  3. 
      
Christian Hammond
  1. 
      
  2. Can you show screenshots/add Testing Done for this on both Firefox (stable) and Chrome?

  3. Can you also test with Local Sites to make sure links are correct?

  4. reviewboard/attachments/models.py (Diff revision 8)
     
     
     

    Should be:

    Yields:
        reviewboard.attachments.models.FileAttachment
        ...
    
  5. reviewboard/static/rb/css/pages/reviews.less (Diff revision 8)
     
     
     
     
     
     
     
     

    Swap these. Elements before classes. (Elements, then IDs, then classes.)

  6. Swap these.

  7. reviewboard/static/rb/css/pages/reviews.less (Diff revision 8)
     
     
     
     

    Two blank lines.

  8. You shouldn't need to use the "localsite" template tag for anything. The url tag from there is automatically injected globally by Review Board.

  9. We want to use the display ID, not the database ID, for review requests on these URLs.

  10. 
      
Barret Rennie
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/attachments/models.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/base.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. reviewboard/reviews/ui/base.py (Diff revision 9)
     
     
    Col: 80
     E501 line too long (117 > 79 characters)
    
  3. 
      
Christian Hammond
  1. 
      
  2. 
      
Barret Rennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (e68e977)
Loading...