flake8
failed.
JSHint
failed.
Mobile Diff Viewer (Diff File Fragments)
Review Request #9364 — Created Nov. 12, 2017 and discarded
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 variableis_mobileto true in thediffviwer/diff_file_fragment.html
template, which applies/shows mobile styles/elements.
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.
| 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 | |
|
E401 multiple imports on one line |
 reviewbot
|
|
|
E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
F401 'djblets.testing.decorators.add_fixtures' imported but unused |
reviewbot
|
|
|
F401 'reviewboard.attachments.tests.BaseFileAttachmentTestCase' imported but unused |
reviewbot
|
|
|
F401 'reviewboard.reviews.models.ReviewRequest' imported but unused |
reviewbot
|
|
|
E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 11 Missing semicolon. |
reviewbot
|
|
|
E999 SyntaxError: invalid syntax |
reviewbot
|
|
|
E122 continuation line missing indentation or outdented |
reviewbot
|
|
|
E251 unexpected spaces around keyword / parameter equals |
reviewbot
|
|
|
E251 unexpected spaces around keyword / parameter equals |
reviewbot
|
|
|
E111 indentation is not a multiple of four |
reviewbot
|
|
|
E113 unexpected indentation |
reviewbot
|
|
|
E225 missing whitespace around operator |
reviewbot
|
|
|
E111 indentation is not a multiple of four |
reviewbot
|
|
|
E225 missing whitespace around operator |
reviewbot
|
|
|
E111 indentation is not a multiple of four |
reviewbot
|
|
|
E225 missing whitespace around operator |
reviewbot
|
|
|
E111 indentation is not a multiple of four |
reviewbot
|
|
|
E225 missing whitespace around operator |
reviewbot
|
|
|
E111 indentation is not a multiple of four |
reviewbot
|
|
|
E225 missing whitespace around operator |
reviewbot
|
|
|
E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
Sort these alphabetically. |
 david
|
|
|
There's enough here that we should really put one arg per line. |
david
|
|
|
Docstring needs Args/Returns sections. |
david
|
|
|
There's an extra blank line here. |
david
|
|
|
The "summary" line of the docstring should be only one line. Docstring also needs Args/Returns. |
david
|
|
|
I know it was like this already, but defining all of these as false and then conditionally assigning them is … |
david
|
|
|
Because we're rendering the whole page with is_mobile in the context, we could avoid adding this and just have the … |
david
|
|
|
There's an extra blank line here. |
david
|
|
|
For the last couple args here, it might be nice to pass them as keyword args so that it's clear … |
david
|
|
|
Add a blank line between these. |
david
|
|
|
The surrounding parentheses aren't necessary here. |
david
|
|
|
We should have a file docstring at the top of this. |
david
|
|
|
This could be condensed a bit: view = self.requset.GET.get('view', 'side-by-side') context.update({ 'is_mobile': (view == 'top-down'), }) |
david
|
|
|
Trailing whitespace. |
david
|
|
|
This could be: {{is_mobile|yesno:'topdown,sidebyside'}} |
david
|
|
|
E251 unexpected spaces around keyword / parameter equals |
reviewbot
|
|
|
E251 unexpected spaces around keyword / parameter equals |
reviewbot
|
|
|
E251 unexpected spaces around keyword / parameter equals |
reviewbot
|
|
|
E251 unexpected spaces around keyword / parameter equals |
reviewbot
|
|
|
E225 missing whitespace around operator |
reviewbot
|
|
|
E226 missing whitespace around arithmetic operator |
reviewbot
|
|
|
E225 missing whitespace around operator |
reviewbot
|
|
|
F841 local variable 'rendered_line_fmt' is assigned to but never used |
reviewbot
|
|
|
W503 line break before binary operator |
reviewbot
|
TB
- 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_mobileto~ Top-down view is acheieved by adding a context variable
is_mobiletodiff_file_fragment.html, which will render diff lines using a differentprintf-formmated template ( mobile_line_fmt). The context variable alsoallows the diff_linestag to perform additional formatting of datait receives before passing it to the template. - Commit:
-
ea0434096ddf1a119e141b252bddd465cdb8f52fdc4fa69aeb8fd41b324cbf3d65ce9ddf1651612d
- Diff:
-
Revision 2 (+500 -158)
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
TB
- Commit:
-
dc4fa69aeb8fd41b324cbf3d65ce9ddf1651612d7539ed1492042dfa64df9b3970dbb1574d132385
- Diff:
-
Revision 3 (+500 -158)
TB
- Commit:
-
7539ed1492042dfa64df9b3970dbb1574d132385b46441707b28c0966c2e800b49a2054e2fe4319a
- Diff:
-
Revision 4 (+502 -158)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
TB
- Commit:
-
b46441707b28c0966c2e800b49a2054e2fe4319abcd62d8ff48f50b1bcf779207ec66bb943a19048
- Diff:
-
Revision 5 (+501 -158)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
-
-
-
-
-
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') -
Because we're rendering the whole page with
is_mobilein the context, we could avoid adding this and just have the template defineline_fmtappropriately in each case. -
-
For the last couple args here, it might be nice to pass them as keyword args so that it's clear what they are.
-
-
-
-
This could be condensed a bit:
view = self.requset.GET.get('view', 'side-by-side') context.update({ 'is_mobile': (view == 'top-down'), }) -
-
TB
- Commit:
-
bcd62d8ff48f50b1bcf779207ec66bb943a190482abb489f86fbf3897dea514887f693fa464d758a
- Diff:
-
Revision 6 (+697 -166)
TB
- Commit:
-
2abb489f86fbf3897dea514887f693fa464d758ae23a544c1587c1d1479c6721ee5010a90ccb76fe
- Diff:
-
Revision 7 (+697 -166)
TB
- Commit:
-
e23a544c1587c1d1479c6721ee5010a90ccb76fea0e3f100d839050161fb5e7b0367205163759c54
- Diff:
-
Revision 8 (+697 -166)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
TB
- Commit:
-
a0e3f100d839050161fb5e7b0367205163759c54869052784e16e752c91e24213bd537b6bcdbf635
- Diff:
-
Revision 9 (+883 -154)
TB
- 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_mobileto~ 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_linestag 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.htmlto render all diff~ fragments. - it receives before passing it to the template. - Commit:
-
869052784e16e752c91e24213bd537b6bcdbf635ea998b197de879029ccb09448c250057cb73dd10
- Diff:
-
Revision 10 (+879 -154)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
TB
- 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.htmlto render all difffragments.
TB
- 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 thetemplate, diffviewer/diff_file_fragment_mobile.htmlto render all difffragments.
TB
- 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.htmlto render all diff~ fragments. ~ of the diff view using query parameter ?view=top-down, which then sets the~ context variable is_mobileto true in thediffviwer/diff_file_fragment.html~ template, which applies/shows mobile styles/elements. - Commit:
-
ea998b197de879029ccb09448c250057cb73dd10f4a4cbf0bf79dc7520a75b1845b6e9d189213e08
- Diff:
-
Revision 11 (+682 -171)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
TB
- Commit:
-
f4a4cbf0bf79dc7520a75b1845b6e9d189213e08042c956877f2f911a117b6b440acb97e5b3766b4
- Diff:
-
Revision 12 (+679 -171)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
TB
- Summary:
-
[WIP] Mobile diff viewer (separated)[WIP] Mobile Diff Viewer (Diff File Fragments)
- Commit:
-
042c956877f2f911a117b6b440acb97e5b3766b4eff87832c394c3cf011d37811c3e1eb5f6d4bcbc
- Diff:
-
Revision 13 (+699 -171)
TB
- Commit:
-
eff87832c394c3cf011d37811c3e1eb5f6d4bcbc0cad1576616c94c0e1925c5d28d9bf2e1d66d5f0
- Diff:
-
Revision 14 (+699 -171)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
TB
- 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.
TB
- Commit:
-
0cad1576616c94c0e1925c5d28d9bf2e1d66d5f079847f9e39eff7f065a0e79b85f393e166bf11b8
- Diff:
-
Revision 15 (+719 -171)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
TB
-
-
Remaining issue:
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.