Mobile Diff Viewer (Diff File Fragments)

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

tbrockma
Review Board
master
79847f9...
reviewboard, students

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.

  • 1
  • 0
  • 47
  • 17
  • 65
Description From Last Updated
Remaining issue: When creating a comment on a "changed" line, there is ambiguity over where the comment will attach. This ... tbrockma
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

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

Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Review request changed
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Review request changed
  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. 
      
Loading...