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

Diff:

Revision 2 (+500 -158)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

TB
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

TB
TB
david
  1. 
      
  2. Sort these alphabetically.

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

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

  4. Docstring needs Args/Returns sections.

  5. There's an extra blank line here.

  6. 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)
     
     
     
     
     
     
     
     
     

    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)
     
     
     
     
     

    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. There's an extra blank line here.

  10. 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. Add a blank line between these.

  12. The surrounding parentheses aren't necessary here.

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

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

    This could be condensed a bit:

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

  16. This could be:

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

TB
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

TB
TB
Review request changed
TB
TB
TB
TB
TB
TB
TB
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

Loading...