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)
Show all issues
```
E401 multiple imports on one line
```
reviewboard/diffviewer/templatetags/difftags.py (Diff revision 1)
Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/templatetags/difftags.py (Diff revision 1)
Show all issues
```
E302 expected 2 blank lines, found 1
```
reviewboard/diffviewer/templatetags/difftags.py (Diff revision 1)
Show all issues
```
E302 expected 2 blank lines, found 1
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
Show all issues
```
F401 'djblets.testing.decorators.add_fixtures' imported but unused
```

reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)

Show all issues

F401 'reviewboard.attachments.tests.BaseFileAttachmentTestCase' imported but unused

reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
Show all issues
```
F401 'reviewboard.reviews.models.ReviewRequest' imported but unused
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
Show all issues
```
E126 continuation line over-indented for hanging indent
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 1)
Show all issues
```
E128 continuation line under-indented for visual indent
```

JSHint

reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js (Diff revision 1)
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)
Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E122 continuation line missing indentation or outdented
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E111 indentation is not a multiple of four
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E113 unexpected indentation
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E225 missing whitespace around operator
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E111 indentation is not a multiple of four
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E225 missing whitespace around operator
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E111 indentation is not a multiple of four
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E225 missing whitespace around operator
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E111 indentation is not a multiple of four
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E225 missing whitespace around operator
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E111 indentation is not a multiple of four
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E225 missing whitespace around operator
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
Show all issues
```
E126 continuation line over-indented for hanging indent
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 2)
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)
Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 3)
Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 3)
Show all issues
```
E126 continuation line over-indented for hanging indent
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 3)
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.

reviewboard/diffviewer/templatetags/difftags.py (Diff revision 5)
Show all issues
```
Sort these alphabetically.
```

reviewboard/diffviewer/templatetags/difftags.py (Diff revision 5)

Show all issues

There's enough here that we should really put one arg per line.

reviewboard/diffviewer/templatetags/difftags.py (Diff revision 5)
Show all issues
```
Docstring needs Args/Returns sections.
```
reviewboard/diffviewer/templatetags/difftags.py (Diff revision 5)
Show all issues
```
There's an extra blank line here.
```

reviewboard/diffviewer/templatetags/difftags.py (Diff revision 5)

Show all issues

The "summary" line of the docstring should be only one line.

Docstring also needs Args/Returns.

reviewboard/diffviewer/templatetags/difftags.py (Diff revision 5)

Show all issues

I know it was like this already, but defining all of these as false and then conditionally assigning them is kind of silly. How about:

is_equal = (change == 'equal')
is_change = (change == 'replace')
is_insert = (change == 'insert')
is_delete = (change == 'delete')

TB

tbrockma Nov. 13, 2017, 9:41 p.m.
```
Yep, I like that a bunch.
```

reviewboard/diffviewer/templatetags/difftags.py (Diff revision 5)

Show all issues

Because we're rendering the whole page with is_mobile in the context, we could avoid adding this and just have the template define line_fmt appropriately in each case.

tbrockma Nov. 13, 2017, 9:41 p.m.

Oh cool, I didn't know if you could do that! Will do.

reviewboard/diffviewer/templatetags/difftags.py (Diff revision 5)
Show all issues
```
There's an extra blank line here.
```

reviewboard/diffviewer/templatetags/difftags.py (Diff revision 5)

Show all issues

For the last couple args here, it might be nice to pass them as keyword args so that it's clear what they are.

reviewboard/diffviewer/templatetags/difftags.py (Diff revision 5)
Show all issues
```
Add a blank line between these.
```
reviewboard/diffviewer/templatetags/difftags.py (Diff revision 5)
Show all issues
```
The surrounding parentheses aren't necessary here.
```
reviewboard/diffviewer/tests/test_diff_fragment_views.py (Diff revision 5)
Show all issues
```
We should have a file docstring at the top of this.
```

reviewboard/diffviewer/views.py (Diff revision 5)

Show all issues

This could be condensed a bit:

view = self.requset.GET.get('view', 'side-by-side')
context.update({
    'is_mobile': (view == 'top-down'),
})

reviewboard/static/rb/css/pages/review-request.less (Diff revision 5)
Show all issues
```
Trailing whitespace.
```

reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 5)

Show all issues

This could be:

{{is_mobile|yesno:'topdown,sidebyside'}}

Commit:

-	bcd62d8ff48f50b1bcf779207ec66bb943a19048
+	2abb489f86fbf3897dea514887f693fa464d758a

Diff:

Revision 6 (+697 -166)

Show changes

	dev-requirements.txt
	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
	4 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/templatetags/difftags.py (Diff revision 6)
Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
reviewboard/diffviewer/templatetags/difftags.py (Diff revision 6)
Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
reviewboard/diffviewer/templatetags/difftags.py (Diff revision 6)
Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
reviewboard/diffviewer/templatetags/difftags.py (Diff revision 6)
Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```

Commit:

-	2abb489f86fbf3897dea514887f693fa464d758a
+	e23a544c1587c1d1479c6721ee5010a90ccb76fe

Diff:

Revision 7 (+697 -166)

Show changes

	dev-requirements.txt
	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
	4 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/templatetags/difftags.py (Diff revision 7)
Show all issues
```
E225 missing whitespace around operator
```
reviewboard/diffviewer/templatetags/difftags.py (Diff revision 7)
Show all issues
```
E226 missing whitespace around arithmetic operator
```
reviewboard/diffviewer/templatetags/difftags.py (Diff revision 7)
Show all issues
```
E225 missing whitespace around operator
```

Commit:

-	e23a544c1587c1d1479c6721ee5010a90ccb76fe
+	a0e3f100d839050161fb5e7b0367205163759c54

Diff:

Revision 8 (+697 -166)

Show changes

	dev-requirements.txt
	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
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

-	a0e3f100d839050161fb5e7b0367205163759c54
+	869052784e16e752c91e24213bd537b6bcdbf635

Diff:

Revision 9 (+883 -154)

Show changes

	dev-requirements.txt
	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/js/diffviewer/models/diffReviewableModel.es6.js
	3 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/templatetags/difftags.py (Diff revision 9)
Show all issues
```
F841 local variable 'rendered_line_fmt' is assigned to but never used
```

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 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
	~	Top-down view is acheieved by allowing client to request an alternate rendering
	~	of the diff view using query parameter "?view=top-down", which then uses the
	~	template, `diffviewer/diff_file_fragment_mobile.html` to render all diff
	~	fragments.
-		it receives before passing it to the template.

Commit:

-	869052784e16e752c91e24213bd537b6bcdbf635
+	ea998b197de879029ccb09448c250057cb73dd10

Diff:

Revision 10 (+879 -154)

Show changes

	dev-requirements.txt
	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/js/diffviewer/models/diffReviewableModel.es6.js
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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 acheieved by allowing client to request an alternate rendering
	~	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 uses the
		template, `diffviewer/diff_file_fragment_mobile.html` to render all diff
		fragments.

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 uses the
	~	of the diff view using query parameter `?view=top-down`, which then uses the
		template, `diffviewer/diff_file_fragment_mobile.html` to render all diff
		fragments.

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 uses the
~		template, `diffviewer/diff_file_fragment_mobile.html` to render all diff
~		fragments.
	~	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.

Commit:

-	ea998b197de879029ccb09448c250057cb73dd10
+	f4a4cbf0bf79dc7520a75b1845b6e9d189213e08

Diff:

Revision 11 (+682 -171)

Show changes

	dev-requirements.txt
	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/js/diffviewer/models/diffReviewableModel.es6.js
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

-	f4a4cbf0bf79dc7520a75b1845b6e9d189213e08
+	042c956877f2f911a117b6b440acb97e5b3766b4

Diff:

Revision 12 (+679 -171)

Show changes

	dev-requirements.txt
	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/js/diffviewer/models/diffReviewableModel.es6.js
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Summary:

-	Mobile diff viewer (separated)
+	[WIP] Mobile diff viewer (separated)

Summary:

-	[WIP] Mobile diff viewer (separated)
+	[WIP] Mobile Diff Viewer (Diff File Fragments)

Commit:

-	042c956877f2f911a117b6b440acb97e5b3766b4
+	eff87832c394c3cf011d37811c3e1eb5f6d4bcbc

Diff:

Revision 13 (+699 -171)

Show changes

	dev-requirements.txt
	reviewboard/diffviewer/renderers.py
	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
	4 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/renderers.py (Diff revision 13)
Show all issues
```
W503 line break before binary operator
```

Commit:

-	eff87832c394c3cf011d37811c3e1eb5f6d4bcbc
+	0cad1576616c94c0e1925c5d28d9bf2e1d66d5f0

Diff:

Revision 14 (+699 -171)

Show changes

	dev-requirements.txt
	reviewboard/diffviewer/renderers.py
	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
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Summary:

-	[WIP] Mobile Diff Viewer (Diff File Fragments)
+	Mobile Diff Viewer (Diff File Fragments)

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 tests asserting correct view is returned for diff fragments with
~		view=top-down query, as well as correct class 'topdown' is applied.
	~	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.

Commit:

-	0cad1576616c94c0e1925c5d28d9bf2e1d66d5f0
+	79847f9e39eff7f065a0e79b85f393e166bf11b8

Diff:

Revision 15 (+719 -171)

Show changes

	dev-requirements.txt
	reviewboard/diffviewer/renderers.py
	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
	5 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Fix it!

Show all issues

Remaining issue:

here
here2

When creating a comment on a "changed" line, there is ambiguity over where the comment will attach. This is because for a change chunk both the original and modified chunk are shown on their own lines, but they do not each have their own comments, and still share comments in between them. I.E from the side-by-side perspective, I am commenting on a single line, but when switching to top-down even though it would seem as if you should be able to comment on either chunk, you are in reality commenting on both, and when making a comment it may either attach to the old chunk or the new one, which may not be necessarily where you initiated the comment. It does not always attach to the old chunk, nor does it always attach to the new.

Mobile Diff Viewer (Diff File Fragments)

flake8

JSHint

Description:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Description:

Commit:

Diff:

Checks run (2 succeeded)

Description:

Description:

Description:

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Summary:

Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (2 succeeded)

Summary:

Testing Done:

Commit:

Diff:

Checks run (2 succeeded)

Status: Discarded