Mobile Diff Viewer (Diff File Fragments)

Review Request #9364 — Created Nov. 12, 2017 and discarded

Information

Review Board
master

Reviewers

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.

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

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

F401 'djblets.testing.decorators.add_fixtures' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

F401 'reviewboard.reviews.models.ReviewRequest' imported but unused

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 11 Missing semicolon.

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E111 indentation is not a multiple of four

reviewbotreviewbot

E113 unexpected indentation

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

E111 indentation is not a multiple of four

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

E111 indentation is not a multiple of four

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

E111 indentation is not a multiple of four

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

E111 indentation is not a multiple of four

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Sort these alphabetically.

daviddavid

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

daviddavid

Docstring needs Args/Returns sections.

daviddavid

There's an extra blank line here.

daviddavid

The "summary" line of the docstring should be only one line. Docstring also needs Args/Returns.

daviddavid

I know it was like this already, but defining all of these as false and then conditionally assigning them is …

daviddavid

Because we're rendering the whole page with is_mobile in the context, we could avoid adding this and just have the …

daviddavid

There's an extra blank line here.

daviddavid

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

daviddavid

Add a blank line between these.

daviddavid

The surrounding parentheses aren't necessary here.

daviddavid

We should have a file docstring at the top of this.

daviddavid

This could be condensed a bit: view = self.requset.GET.get('view', 'side-by-side') context.update({ 'is_mobile': (view == 'top-down'), })

daviddavid

Trailing whitespace.

daviddavid

This could be: {{is_mobile|yesno:'topdown,sidebyside'}}

daviddavid

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

E226 missing whitespace around arithmetic operator

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

F841 local variable 'rendered_line_fmt' is assigned to but never used

reviewbotreviewbot

W503 line break before binary operator

reviewbotreviewbot
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

TB
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

TB
Review request changed
Commit:
dc4fa69aeb8fd41b324cbf3d65ce9ddf1651612d
7539ed1492042dfa64df9b3970dbb1574d132385

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

TB
TB
david
  1. 
      
  2. Show all issues

    Sort these alphabetically.

  3. reviewboard/diffviewer/templatetags/difftags.py (Diff revision 5)
     
     
     
     
     
     
     
     
    Show all issues

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

  4. Show all issues

    Docstring needs Args/Returns sections.

  5. Show all issues

    There's an extra blank line here.

  6. Show all issues

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

    Docstring also needs Args/Returns.

  7. 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')
    
    1. Yep, I like that a bunch.

  8. 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.

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

  9. Show all issues

    There's an extra blank line here.

  10. 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.

  11. Show all issues

    Add a blank line between these.

  12. Show all issues

    The surrounding parentheses aren't necessary here.

  13. Show all issues

    We should have a file docstring at the top of this.

  14. 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'),
    })
    
  15. Show all issues

    Trailing whitespace.

  16. Show all issues

    This could be:

    {{is_mobile|yesno:'topdown,sidebyside'}}
    
  17. 
      
TB
Review request changed
Commit:
bcd62d8ff48f50b1bcf779207ec66bb943a19048
2abb489f86fbf3897dea514887f693fa464d758a

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

TB
Review request changed
Commit:
2abb489f86fbf3897dea514887f693fa464d758a
e23a544c1587c1d1479c6721ee5010a90ccb76fe

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

TB
TB
Review request changed
Commit:
a0e3f100d839050161fb5e7b0367205163759c54
869052784e16e752c91e24213bd537b6bcdbf635

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

TB
TB
TB
TB
TB
TB
TB
Review request changed
Summary:
[WIP] Mobile diff viewer (separated)
[WIP] Mobile Diff Viewer (Diff File Fragments)
Commit:
042c956877f2f911a117b6b440acb97e5b3766b4
eff87832c394c3cf011d37811c3e1eb5f6d4bcbc

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

TB
TB
TB
TB
TB
  1. 
      
  2. 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.

  3. 
      
david
Review request changed
Status:
Discarded