Mobile diff viewer (separated)

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

Theodore Brockman
Review Board
master
a0e3f10...
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 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.

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.

  • 1
  • 0
  • 44
  • 17
  • 62
Description From Last Updated
Because we're rendering the whole page with is_mobile in the context, we could avoid adding this and just have the ... David Trowbridge David Trowbridge
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

Theodore Brockman
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

Theodore Brockman
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Theodore Brockman
Theodore Brockman
David Trowbridge
  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. 
      
Theodore Brockman
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Theodore Brockman
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Theodore Brockman
Review request changed
Loading...