Add previous and next buttons to navigate between file attachments
Review Request #8443 — Created Sept. 27, 2016 and submitted
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.
| Description | From | Last Updated |
|---|---|---|
|
Can you show screenshots/add Testing Done for this on both Firefox (stable) and Chrome? |
 chipx86
|
|
|
Can you also test with Local Sites to make sure links are correct? |
chipx86
|
|
|
Can we give this a bit of a box-shadow so that the layering is more apparent? |
 david
|
|
|
It might be nice to break this out into a method, or at least comment it and explain what it's … |
david
|
|
|
Perhaps we can change get_latest_file_attachments to return a generator expression instead of a list? |
david
|
|
|
I don't think this change is correct. |
david
|
|
|
Where does the 14px come from? |
david
|
|
|
There's an extra space before this templatetag. |
david
|
|
|
This is all indented 1 space too much. |
david
|
|
|
Indented 1 space too much. |
david
|
|
|
undefined name 'next_obj' |
 reviewbot
|
|
|
undefined name 'prev_obj' |
reviewbot
|
|
|
Col: 80 E501 line too long (117 > 79 characters) |
reviewbot
|
|
|
It might be nice to say what "next" and "previous" mean. |
david
|
|
|
Col: 80 E501 line too long (117 > 79 characters) |
reviewbot
|
|
|
Can we define a constant for this? |
david
|
|
|
Col: 80 E501 line too long (117 > 79 characters) |
reviewbot
|
|
|
If we made a change to have @page-container-padding be in px instead of em, we could use LESS's calculations here … |
david
|
|
|
Col: 80 E501 line too long (117 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (117 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (117 > 79 characters) |
reviewbot
|
|
|
Should be: Yields: reviewboard.attachments.models.FileAttachment ... |
chipx86
|
|
|
Col: 80 E501 line too long (117 > 79 characters) |
reviewbot
|
|
|
Swap these. Elements before classes. (Elements, then IDs, then classes.) |
chipx86
|
|
|
Swap these. |
chipx86
|
|
|
Two blank lines. |
chipx86
|
|
|
You shouldn't need to use the "localsite" template tag for anything. The url tag from there is automatically injected globally … |
chipx86
|
|
|
We want to use the display ID, not the database ID, for review requests on these URLs. |
chipx86
|
|
|
Col: 80 E501 line too long (117 > 79 characters) |
reviewbot
|
-
-
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.
-
reviewboard/reviews/ui/base.py (Diff revision 1) Perhaps we can change
get_latest_file_attachmentsto return a generator expression instead of a list? -
-
-
reviewboard/templates/reviews/ui/base.html (Diff revision 1) There's an extra space before this templatetag.
-
-
Change Summary:
Addressed David's issues. Made the code more compatible with IE
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+162 -22) |
-
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 -
-
-
Change Summary:
Cleanup.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+162 -22) |
-
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 -
-
-
-
reviewboard/reviews/ui/base.py (Diff revision 3) It might be nice to say what "next" and "previous" mean.
Change Summary:
Addressed David's issues.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+166 -22) |
-
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 -
-
-
reviewboard/static/rb/css/pages/reviews.less (Diff revisions 3 - 4) Can we define a constant for this?
Change Summary:
Addressed David's issue
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+166 -22) |
-
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 -
-
-
reviewboard/static/rb/css/pages/reviews.less (Diff revisions 4 - 5) If we made a change to have
@page-container-paddingbe inpxinstead ofem, we could use LESS's calculations here instead ofcalc().
Change Summary:
Fix attachment order and variable interpolation
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+166 -22) |
||||
Removed Files: |
-
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 -
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 7 (+166 -22) |
-
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 -
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+167 -22) |
-
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 -
-
-
-
-
reviewboard/attachments/models.py (Diff revision 8) Should be:
Yields: reviewboard.attachments.models.FileAttachment ... -
reviewboard/static/rb/css/pages/reviews.less (Diff revision 8) Swap these. Elements before classes. (Elements, then IDs, then classes.)
-
-
-
reviewboard/templates/reviews/ui/base.html (Diff revision 8) You shouldn't need to use the "localsite" template tag for anything. The
urltag from there is automatically injected globally by Review Board. -
reviewboard/templates/reviews/ui/base.html (Diff revision 8) We want to use the display ID, not the database ID, for review requests on these URLs.
Testing Done: |
|
||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||||||||||||||
Diff: |
Revision 9 (+167 -21) |
||||||||||||||||||||||||||||||||||||
Removed Files: |
|||||||||||||||||||||||||||||||||||||
Added Files: |
-
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 -
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