Summary

Mobile Diff Viewer (Diff File Fragments)

Review Request #9364 — Created Nov. 12, 2017 and discarded Feb. 10, 2019, 9:16 a.m.

Information

Owner

tbrockma

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

Mobile diff viewer, allows for a top-down rendering of diff fragments which is
friendlier for mobile devices. Contains a bunch of CSS stylings only to be
applied when screensize <= 720px, most of which aim to eliminate wasted space.

Top-down view is achieved by allowing client to request an alternate rendering
of the diff view using query parameter ?view=top-down, which then sets the
context variable is_mobile to true in the diffviwer/diff_file_fragment.html
template, which applies/shows mobile styles/elements.

Testing Done

Beautifulsoup was included as a dev-requirement for adding some CSS-selector-
like functionality within Python tests, so that specific elements of the
templates returned by Django could be tested while still having the tests
purpose be explicit and comprehensible.

Includes javascript test to assert correct AJAX request is made for top-down
diff view on when screenwidth <= 720px.
Also has some Python tests asserting correct view is returned for diff fragments
with view=top-down query, as well as correct classes and stylings are applied.

Issues

Description	From	Last Updated
Remaining issue: When creating a comment on a "changed" line, there is ambiguity over where the comment will attach. This …	TB tbrockma	Dec. 10, 2017, 8:18 p.m.
E401 multiple imports on one line	reviewbot	Nov. 12, 2017, 12:23 a.m.
E501 line too long (80 > 79 characters)	reviewbot	Nov. 12, 2017, 12:23 a.m.
E302 expected 2 blank lines, found 1	reviewbot	Nov. 12, 2017, 12:23 a.m.
E302 expected 2 blank lines, found 1	reviewbot	Nov. 12, 2017, 12:23 a.m.
F401 'djblets.testing.decorators.add_fixtures' imported but unused	reviewbot	Nov. 12, 2017, 12:23 a.m.
F401 'reviewboard.attachments.tests.BaseFileAttachmentTestCase' imported but unused	reviewbot	Nov. 12, 2017, 12:23 a.m.
F401 'reviewboard.reviews.models.ReviewRequest' imported but unused	reviewbot	Nov. 12, 2017, 12:23 a.m.
E501 line too long (80 > 79 characters)	reviewbot	Nov. 12, 2017, 12:23 a.m.
E126 continuation line over-indented for hanging indent	reviewbot	Nov. 12, 2017, 12:23 a.m.
E501 line too long (80 > 79 characters)	reviewbot	Nov. 12, 2017, 12:23 a.m.
E501 line too long (80 > 79 characters)	reviewbot	Nov. 12, 2017, 12:23 a.m.
E501 line too long (80 > 79 characters)	reviewbot	Nov. 12, 2017, 12:23 a.m.
E501 line too long (80 > 79 characters)	reviewbot	Nov. 12, 2017, 12:23 a.m.
E501 line too long (80 > 79 characters)	reviewbot	Nov. 12, 2017, 12:23 a.m.
E128 continuation line under-indented for visual indent	reviewbot	Nov. 12, 2017, 12:23 a.m.
E128 continuation line under-indented for visual indent	reviewbot	Nov. 12, 2017, 12:23 a.m.
Col: 11 Missing semicolon.	reviewbot	Nov. 12, 2017, 12:21 a.m.
E999 SyntaxError: invalid syntax	reviewbot	Nov. 12, 2017, 12:33 a.m.
E122 continuation line missing indentation or outdented	reviewbot	Nov. 12, 2017, 12:33 a.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	Nov. 12, 2017, 12:33 a.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	Nov. 12, 2017, 12:33 a.m.
E111 indentation is not a multiple of four	reviewbot	Nov. 12, 2017, 12:33 a.m.
E113 unexpected indentation	reviewbot	Nov. 12, 2017, 12:33 a.m.
E225 missing whitespace around operator	reviewbot	Nov. 12, 2017, 12:33 a.m.
E111 indentation is not a multiple of four	reviewbot	Nov. 12, 2017, 12:33 a.m.
E225 missing whitespace around operator	reviewbot	Nov. 12, 2017, 12:33 a.m.
E111 indentation is not a multiple of four	reviewbot	Nov. 12, 2017, 12:33 a.m.
E225 missing whitespace around operator	reviewbot	Nov. 12, 2017, 12:33 a.m.
E111 indentation is not a multiple of four	reviewbot	Nov. 12, 2017, 12:33 a.m.
E225 missing whitespace around operator	reviewbot	Nov. 12, 2017, 12:33 a.m.
E111 indentation is not a multiple of four	reviewbot	Nov. 12, 2017, 12:33 a.m.
E225 missing whitespace around operator	reviewbot	Nov. 12, 2017, 12:33 a.m.
E501 line too long (80 > 79 characters)	reviewbot	Nov. 12, 2017, 12:33 a.m.
E501 line too long (80 > 79 characters)	reviewbot	Nov. 12, 2017, 12:33 a.m.
E126 continuation line over-indented for hanging indent	reviewbot	Nov. 12, 2017, 12:33 a.m.
E126 continuation line over-indented for hanging indent	reviewbot	Nov. 12, 2017, 12:33 a.m.
E501 line too long (80 > 79 characters)	reviewbot	Nov. 12, 2017, 12:33 a.m.
E501 line too long (80 > 79 characters)	reviewbot	Nov. 12, 2017, 12:33 a.m.
E126 continuation line over-indented for hanging indent	reviewbot	Nov. 12, 2017, 12:34 a.m.
E126 continuation line over-indented for hanging indent	reviewbot	Nov. 12, 2017, 12:34 a.m.
Sort these alphabetically.	david	Nov. 13, 2017, 9:41 p.m.
There's enough here that we should really put one arg per line.	david	Nov. 13, 2017, 9:53 p.m.
Docstring needs Args/Returns sections.	david	Nov. 14, 2017, 12:49 a.m.
There's an extra blank line here.	david	Nov. 13, 2017, 9:52 p.m.
The "summary" line of the docstring should be only one line. Docstring also needs Args/Returns.	david	Nov. 14, 2017, 12:49 a.m.
I know it was like this already, but defining all of these as false and then conditionally assigning them is …	david	Nov. 13, 2017, 9:41 p.m.
Because we're rendering the whole page with is_mobile in the context, we could avoid adding this and just have the …	david	Nov. 26, 2017, 4:35 p.m.
There's an extra blank line here.	david	Nov. 14, 2017, 12:06 a.m.
For the last couple args here, it might be nice to pass them as keyword args so that it's clear …	david	Nov. 13, 2017, 11:57 p.m.
Add a blank line between these.	david	Nov. 13, 2017, 9:51 p.m.
The surrounding parentheses aren't necessary here.	david	Nov. 13, 2017, 9:51 p.m.
We should have a file docstring at the top of this.	david	Nov. 14, 2017, 12:06 a.m.
This could be condensed a bit: view = self.requset.GET.get('view', 'side-by-side') context.update({ 'is_mobile': (view == 'top-down'), })	david	Nov. 13, 2017, 9:50 p.m.
Trailing whitespace.	david	Nov. 13, 2017, 11:57 p.m.
This could be: {{is_mobile\|yesno:'topdown,sidebyside'}}	david	Nov. 13, 2017, 9:50 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	Nov. 14, 2017, 12:58 a.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	Nov. 14, 2017, 12:58 a.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	Nov. 14, 2017, 12:58 a.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	Nov. 14, 2017, 12:58 a.m.
E225 missing whitespace around operator	reviewbot	Nov. 14, 2017, 12:58 a.m.
E226 missing whitespace around arithmetic operator	reviewbot	Nov. 14, 2017, 12:58 a.m.
E225 missing whitespace around operator	reviewbot	Nov. 14, 2017, 12:58 a.m.
F841 local variable 'rendered_line_fmt' is assigned to but never used	reviewbot	Nov. 26, 2017, 4:35 p.m.
W503 line break before binary operator	reviewbot	Nov. 26, 2017, 10:46 p.m.

flake8 failed.

JSHint failed.

flake8

reviewboard/diffviewer/templatetags/difftags.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E401 multiple imports on one line
```
reviewboard/diffviewer/templatetags/difftags.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/templatetags/difftags.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E302 expected 2 blank lines, found 1
```
reviewboard/diffviewer/templatetags/difftags.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E302 expected 2 blank lines, found 1
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'djblets.testing.decorators.add_fixtures' imported but unused
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'reviewboard.attachments.tests.BaseFileAttachmentTestCase' imported but unused
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'reviewboard.reviews.models.ReviewRequest' imported but unused
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E126 continuation line over-indented for hanging indent
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E128 continuation line under-indented for visual indent
```

JSHint

reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 11
 Missing semicolon.
```

Description:

		Mobile diff viewer, allows for a top-down rendering of diff fragments which is
		friendlier for mobile devices. Contains a bunch of CSS stylings only to be
		applied when screensize <= 720px, most of which aim to eliminate wasted space.

~		This is acheieved by adding a context variable `is_mobile` to
	~	Top-down view is acheieved by adding a context variable `is_mobile` to
		`diff_file_fragment.html`, which will render diff lines using a different
		printf-formmated template (`mobile_line_fmt`). The context variable also
		allows the `diff_lines` tag to perform additional formatting of data
		it receives before passing it to the template.

Commit:

-	ea0434096ddf1a119e141b252bddd465cdb8f52f
+	dc4fa69aeb8fd41b324cbf3d65ce9ddf1651612d

Diff:

Revision 2 (+500 -158)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/templatetags/difftags.py
	reviewboard/diffviewer/tests/test_diff_fragment_views.py
	reviewboard/static/rb/css/pages/base.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js
	3 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E122 continuation line missing indentation or outdented
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E111 indentation is not a multiple of four
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E113 unexpected indentation
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E225 missing whitespace around operator
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E111 indentation is not a multiple of four
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E225 missing whitespace around operator
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E111 indentation is not a multiple of four
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E225 missing whitespace around operator
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E111 indentation is not a multiple of four
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E225 missing whitespace around operator
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E111 indentation is not a multiple of four
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E225 missing whitespace around operator
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E126 continuation line over-indented for hanging indent
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E126 continuation line over-indented for hanging indent
```

Commit:

-	dc4fa69aeb8fd41b324cbf3d65ce9ddf1651612d
+	7539ed1492042dfa64df9b3970dbb1574d132385

Diff:

Revision 3 (+500 -158)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/templatetags/difftags.py
	reviewboard/diffviewer/tests/test_diff_fragment_views.py
	reviewboard/static/rb/css/pages/base.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js
	3 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E126 continuation line over-indented for hanging indent
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E126 continuation line over-indented for hanging indent
```

Commit:

-	7539ed1492042dfa64df9b3970dbb1574d132385
+	b46441707b28c0966c2e800b49a2054e2fe4319a

Diff:

Revision 4 (+502 -158)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/templatetags/difftags.py
	reviewboard/diffviewer/tests/test_diff_fragment_views.py
	reviewboard/static/rb/css/pages/base.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

-	b46441707b28c0966c2e800b49a2054e2fe4319a
+	bcd62d8ff48f50b1bcf779207ec66bb943a19048

Diff:

Revision 5 (+501 -158)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/templatetags/difftags.py
	reviewboard/diffviewer/tests/test_diff_fragment_views.py
	reviewboard/static/rb/css/pages/base.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.