Summary

Add previous and next buttons to navigate between file attachments

Review Request #8443 — Created Sept. 27, 2016 and submitted Oct. 25, 2016, 9:51 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

0992759...

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

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.

Files

Issues

Description	From	Last Updated
Can you show screenshots/add Testing Done for this on both Firefox (stable) and Chrome?	chipx86	Oct. 13, 2016, 2:32 p.m.
Can you also test with Local Sites to make sure links are correct?	chipx86	Oct. 13, 2016, 2:32 p.m.
Can we give this a bit of a box-shadow so that the layering is more apparent?	david	Oct. 6, 2016, 12:57 p.m.
It might be nice to break this out into a method, or at least comment it and explain what it's …	david	Sept. 29, 2016, 3:41 p.m.
Perhaps we can change get_latest_file_attachments to return a generator expression instead of a list?	david	Sept. 29, 2016, 3:35 p.m.
I don't think this change is correct.	david	Sept. 29, 2016, 3:09 p.m.
Where does the 14px come from?	david	Sept. 29, 2016, 3:35 p.m.
There's an extra space before this templatetag.	david	Sept. 29, 2016, 3:41 p.m.
This is all indented 1 space too much.	david	Sept. 29, 2016, 3:41 p.m.
Indented 1 space too much.	david	Sept. 29, 2016, 3:41 p.m.
undefined name 'next_obj'	reviewbot	Sept. 29, 2016, 6:29 p.m.
undefined name 'prev_obj'	reviewbot	Sept. 29, 2016, 6:29 p.m.
Col: 80 E501 line too long (117 > 79 characters)	reviewbot	Sept. 29, 2016, 6:29 p.m.
It might be nice to say what "next" and "previous" mean.	david	Oct. 6, 2016, 12:57 p.m.
Col: 80 E501 line too long (117 > 79 characters)	reviewbot	Sept. 29, 2016, 6:35 p.m.
Can we define a constant for this?	david	Oct. 6, 2016, 5:18 p.m.
Col: 80 E501 line too long (117 > 79 characters)	reviewbot	Oct. 6, 2016, 2:26 p.m.
If we made a change to have @page-container-padding be in px instead of em, we could use LESS's calculations here …	david	Oct. 7, 2016, 3:45 p.m.
Col: 80 E501 line too long (117 > 79 characters)	reviewbot	Oct. 6, 2016, 5:19 p.m.
Col: 80 E501 line too long (117 > 79 characters)	reviewbot	Oct. 7, 2016, 1:49 p.m.
Col: 80 E501 line too long (117 > 79 characters)	reviewbot	Oct. 7, 2016, 2:32 p.m.
Should be: Yields: reviewboard.attachments.models.FileAttachment ...	chipx86	Oct. 12, 2016, 2:15 p.m.
Col: 80 E501 line too long (117 > 79 characters)	reviewbot	Oct. 7, 2016, 11:29 p.m.
Swap these. Elements before classes. (Elements, then IDs, then classes.)	chipx86	Oct. 12, 2016, 2:16 p.m.
Swap these.	chipx86	Oct. 12, 2016, 2:16 p.m.
Two blank lines.	chipx86	Oct. 12, 2016, 2:16 p.m.
You shouldn't need to use the "localsite" template tag for anything. The url tag from there is automatically injected globally …	chipx86	Oct. 12, 2016, 2:16 p.m.
We want to use the display ID, not the database ID, for review requests on these URLs.	chipx86	Oct. 12, 2016, 2:17 p.m.
Col: 80 E501 line too long (117 > 79 characters)	reviewbot	Oct. 13, 2016, 2:50 p.m.

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 1)

The issue has been resolved. Show all issues

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)

The issue has been resolved. Show all issues

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

reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
I don't think this change is correct.
```
1. brennie Sept. 29, 2016, 3:41 p.m.
  Thanks for catching that.
reviewboard/static/rb/css/pages/reviews.less (Diff revision 1)
The issue has been resolved. Show all issues
```
Where does the 14px come from?
```
reviewboard/templates/reviews/ui/base.html (Diff revision 1)
The issue has been resolved. Show all issues
```
There's an extra space before this templatetag.
```

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

The issue has been resolved. Show all issues

This is all indented 1 space too much.

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

The issue has been resolved. Show all issues

Indented 1 space too much.

Change Summary:

Addressed David's issues. Made the code more compatible with IE

Commit:

305608c07b673d00cbcf99b8ce44d360d3ab8d3d

b9b11aea2e630ba0aabb11367a0f6a701042f93f

Diff:

Revision 2 (+162 -22)

Show changes

	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/ui/base.html

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 2)
The issue has been resolved. Show all issues
```
 undefined name 'next_obj'
```
reviewboard/reviews/ui/base.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 undefined name 'prev_obj'
```
reviewboard/reviews/ui/base.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```

Change Summary:

Cleanup.

Commit:

b9b11aea2e630ba0aabb11367a0f6a701042f93f

49994c758a8899b6c937f07e83f0104b37a12c60

Diff:

Revision 3 (+162 -22)

Show changes

	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/ui/base.html

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)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```

prev_expanded.png (Revision 1)

The issue has been resolved. Show all issues

Can we give this a bit of a box-shadow so that the layering is more apparent?

reviewboard/reviews/ui/base.py (Diff revision 3)
The issue has been resolved. Show all issues
```
It might be nice to say what "next" and "previous" mean.
```

Change Summary:

Addressed David's issues.

Commit:

49994c758a8899b6c937f07e83f0104b37a12c60

e38efba14f7a8bdfad7e580830bc4b3339733dd0

Diff:

Revision 4 (+166 -22)

Show changes

	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/ui/base.html

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 4)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```

reviewboard/static/rb/css/pages/reviews.less (Diff revisions 3 - 4)
The issue has been resolved. Show all issues
```
Can we define a constant for this?
```
1. brennie Oct. 6, 2016, 5:18 p.m.
  Replaced this with a calc() expression.

Change Summary:

Addressed David's issue

Commit:

e38efba14f7a8bdfad7e580830bc4b3339733dd0

f3d456a53e9cea1547c6ed17b8b397dd1d89c450

Diff:

Revision 5 (+166 -22)

Show changes

	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/ui/base.html

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 5)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```

reviewboard/static/rb/css/pages/reviews.less (Diff revisions 4 - 5)

The issue has been resolved. Show all issues

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().

Change Summary:

Fix attachment order and variable interpolation

Commit:

f3d456a53e9cea1547c6ed17b8b397dd1d89c450

b15785ccb57e0389347aac9c12607e477baf15a6

Diff:

Revision 6 (+166 -22)

Show changes

	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/ui/base.html

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

reviewboard/reviews/ui/base.py (Diff revision 6)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```

Commit:

b15785ccb57e0389347aac9c12607e477baf15a6

c8889b753c656305c53afbad26df97b913072570

Diff:

Revision 7 (+166 -22)

Show changes

	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/ui/base.html

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 7)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```

Change Summary:: Refactored out CSS calc() expression.
Decreased padding and font size of chevrons so they dont go off the sides of the page.
Removed Files:
Added Files:

Commit:

c8889b753c656305c53afbad26df97b913072570

b0f839e110f0b23fd2f61c9e10c212161cfe7cfd

Diff:

Revision 8 (+167 -22)

Show changes

	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/ui/base.html

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 8)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```

The issue has been resolved. Show all issues

Can you show screenshots/add Testing Done for this on both Firefox (stable) and Chrome?

The issue has been resolved. Show all issues

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

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

The issue has been resolved. Show all issues

Should be:

Yields:
    reviewboard.attachments.models.FileAttachment
    ...

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

The issue has been resolved. Show all issues

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

reviewboard/static/rb/css/pages/reviews.less (Diff revision 8)
The issue has been resolved. Show all issues
```
Swap these.
```
reviewboard/static/rb/css/pages/reviews.less (Diff revision 8)
The issue has been resolved. Show all issues
```
Two blank lines.
```

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

The issue has been resolved. Show all issues

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

reviewboard/templates/reviews/ui/base.html (Diff revision 8)
The issue has been resolved. Show all issues
```
We want to use the display ID, not the database ID, for review requests on these URLs.
```

Testing Done:

~		Ran unit tests.
~		Navigated between file attachments. Saw that navigation occurred correctly.
	~	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.

Commit:

b0f839e110f0b23fd2f61c9e10c212161cfe7cfd

0992759f3e75e6ec483b4c999e15452798cd7e61

Diff:

Revision 9 (+167 -21)

Show changes

	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/ui/base.html

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

reviewboard/reviews/ui/base.py (Diff revision 9)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```

Ship it!

Status:: Completed
Change Summary:: Pushed to release-3.0.x (e68e977)