File Attachment Revision Slider
Review Request #6591 — Created Nov. 15, 2014 and submitted
The slider is now functional for image attachments and text attachments (including rendered text), initiating the controls for different diff modes. The slider actions trigger a hard redirect for now (window.location.replace).
With enough time I may be able to do a soft redirect using router and ajax (like _loadRevision in diffViewerPageView.js)The URL argument is what is used to specify the files being diffed. ".../r/<rrID>/<FileAttachmentID>-<DiffID>/"
The first option in the slider is the No Diff option, which will redirect to
.../r/<rrID>/<FileAttachmentID>/
This is how switching back to single files works.
This mirrors the way the slider works in review requests. (diffRevisionSelectorView.js).A title has been added to the slider that behaves the same as the title for the diffviewer revision slider. The class implementation is more or less the same except is refers to the fileAttachmentRevisionModel instead of the diffRevisionModel.
Clicking on labels other than the no diff will redirect to that single file view.
I'm having trouble changing the text attachment title header to have each file title allign with their respective file, so for now they are just spaced out with an & symbol.
TODO:
- Text File title allignment (columns?)
- CSS Spacing issues?For another Review Request
- Adding coverage to unit tests
- soft redirect
Verified by hand that url argument <ID>-<diffID> throws a 404 if the two IDs do not share attachment history for both images and text attachments.
All unit tests pass
Manually tested the slider for text, markdown and images.
| Description | From | Last Updated |
|---|---|---|
|
Why does the revision slider make the footer of the table beige? Can it be modified so that the footer … |
 brennie
|
|
|
Col: 80 E501 line too long (188 > 79 characters) |
 reviewbot
|
|
|
Should only have one var statement. |
 chipx86
|
|
|
Spaces around the -. |
chipx86
|
|
|
This would be easier to read if you only did the conditional once, and computed the proper set of values … |
chipx86
|
|
|
Here too. |
chipx86
|
|
|
I don't think the default UI should be deciding where this goes, or should even place it anywhere. It should … |
chipx86
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
We're doing this twice. Best to get the results once, convert to a list, then use it both here and … |
chipx86
|
|
|
Alphabetical order. |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
Indentation error on the mark_safe line. |
chipx86
|
|
|
And between these. |
chipx86
|
|
|
Blank line here. |
chipx86
|
|
|
And here. |
chipx86
|
|
|
Can you add comments on exactly what's happening here? It's unclear. |
chipx86
|
|
|
Blank line here. |
chipx86
|
|
|
We're doing two requests needlessly here, and database requests are expensive. I think we should just do a get_object_or_404 with … |
chipx86
|
|
|
Arguments should align. |
chipx86
|
|
|
This is a bit long. How about just _revisionSelectorView? |
chipx86
|
|
|
Arguments must align. |
chipx86
|
|
|
Looks like a missing ` on base. |
chipx86
|
|
|
No blank line here. |
chipx86
|
|
|
This should go before the variables without values. |
chipx86
|
|
|
Blank line here. |
chipx86
|
|
|
Multi-line comments should use the: /* * */ form. |
chipx86
|
|
|
Let's compute a URL once, then use window.location.replace(). |
chipx86
|
|
|
Same comment about the name. |
chipx86
|
|
|
Same comment about alignment. Above comments also all apply to this file as well. |
chipx86
|
RM
Change Summary:
Properly implemented the setup function so that the slider now reflects the number of revisions associated with a fileAttachment.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+35 -6) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/ui/base.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/templates/reviews/ui/default.html reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js Tool: Pyflakes Processed Files: reviewboard/reviews/ui/base.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/templates/reviews/ui/default.html reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
-
-
reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js (Diff revision 2) Should only have one
varstatement. -
reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js (Diff revision 2) Spaces around the
-. -
reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js (Diff revision 2) This would be easier to read if you only did the conditional once, and computed the proper set of values for
this._valuesonce depending on that. -
-
-
reviewboard/templates/reviews/ui/default.html (Diff revision 2) I don't think the default UI should be deciding where this goes, or should even place it anywhere. It should be up to the specific review UI implementation to create an element for use in the revision slider.
RM
Change Summary:
Addressed style issues, moved html div for the slider from default.html to imageReviewableView.js
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+44 -6) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/ui/base.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js Tool: Pyflakes Processed Files: reviewboard/reviews/ui/base.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
RM
Change Summary:
WIP code for adding a column to a text file attachment template for a 2nd file
Added revisionSelected Callbacks for the slider in both image and text file file attachment views
Added a slider render to the text attachment (this one is declared in the text.html template)
Description: |
|
||||||
|---|---|---|---|---|---|---|---|
Commit: |
|
||||||
Diff: |
Revision 4 (+100 -8) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/ui/base.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/js/views/textBasedReviewableView.js Tool: Pyflakes Processed Files: reviewboard/reviews/ui/base.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/js/views/textBasedReviewableView.js
RM
Change Summary:
Added image diff slider that uses hard redirects through URL changes.
Description: |
|
|||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||
Diff: |
Revision 5 (+129 -9) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/urls.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/js/views/textBasedReviewableView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/urls.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/js/views/textBasedReviewableView.js
RM
Description: |
|
||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Added Files: |
RM
Change Summary:
Added text attachment slider
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 6 (+153 -15) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/ui/text.py reviewboard/reviews/urls.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/js/views/textBasedReviewableView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/ui/text.py reviewboard/reviews/urls.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/js/views/textBasedReviewableView.js
RM
Change Summary:
Added markdown diffing
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 7 (+181 -21) |
||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/ui/text.py reviewboard/reviews/urls.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/js/views/textBasedReviewableView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/ui/text.py reviewboard/reviews/urls.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/js/views/textBasedReviewableView.js -
RM
Description: |
|
|---|
RM
Change Summary:
Moving out of WIP
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
RM
Description: |
|
|---|
RM
Change Summary:
Changed slider behaviour to mirror the diff revision slider in review requests and slider position to above attachment.
Added new screenshots
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 8 (+210 -22) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Removed Files: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/ui/text.py reviewboard/reviews/urls.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/ui/text.py reviewboard/reviews/urls.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
-
-
reviewboard/reviews/ui/base.py (Diff revision 8) We're doing this twice. Best to get the results once, convert to a list, then use it both here and for the IDs list.
-
-
-
-
-
-
-
reviewboard/reviews/ui/text.py (Diff revision 8) Can you add comments on exactly what's happening here? It's unclear.
-
-
reviewboard/reviews/views.py (Diff revision 8) We're doing two requests needlessly here, and database requests are expensive. I think we should just do a
get_object_or_404with the entire query in theifstatement. -
reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js (Diff revision 8) Arguments should align.
-
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 8) This is a bit long. How about just
_revisionSelectorView? -
-
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 8) Looks like a missing ` on base.
-
-
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 8) This should go before the variables without values.
-
-
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 8) Multi-line comments should use the:
/* * */form.
-
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 8) Let's compute a URL once, then use
window.location.replace(). -
reviewboard/static/rb/js/views/textBasedReviewableView.js (Diff revision 8) Same comment about the name.
-
reviewboard/static/rb/js/views/textBasedReviewableView.js (Diff revision 8) Same comment about alignment.
Above comments also all apply to this file as well.
RM
Change Summary:
Addressed changes in Christians review, and updated title display for files when it diff mode (the Title now shows both file titles and captions).
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 9 (+259 -25) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/ui/text.py reviewboard/reviews/urls.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/ui/text.py reviewboard/reviews/urls.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
RM
Change Summary:
Added a slider title that behaves the same as the diffRevisionLabel that works with the diffRevisionSelectorView.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 10 (+369 -25) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/ui/text.py reviewboard/reviews/urls.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js reviewboard/static/rb/js/views/fileAttachmentRevisionLabelView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/ui/text.py reviewboard/reviews/urls.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js reviewboard/static/rb/js/views/fileAttachmentRevisionLabelView.js
RM
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
Tool: Pyflakes Processed Files: reviewboard/reviews/ui/base.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/templates/reviews/ui/default.html reviewboard/static/rb/js/views/revisionSelectorView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/ui/base.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/templates/reviews/ui/default.html reviewboard/static/rb/js/views/revisionSelectorView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js