Add a mobile view for the diff viewer.

Review Request #13312 — Created Oct. 6, 2023 and updated

Information

Review Board
master

Reviewers

This introduces a long-awaited mobile view for the diff viewer. This is
a pure-CSS solution for viewing existing diffs in a mobile, unified diff
display, without the need for a re-render of a diff.

All diff functionality works as normally. Users can browse the diff,
expand, collapse, download files, make multi-line comments,
view/navigate moved line flags, and view interdiffs (though the revision
slider has interaction problems on mobile).

On mobile, the diff viewer is rendered as a CSS Grid, with three
columns: Original line number, modified line number, and line content.

The grid's column setup provides ample room for large line numbers
(safely up to 6 digits) before a given row's line number column
truncates (it will truncate on the left). This is for browsers without
support for Subgrids. For those that do support Subgrids, the columns
will resize for the number of digits needed (and reserve less space to
start).

Inserts, deletes, and equals present the same as before, though only one
column of line content is shown. Replaces work differently, as they're
shown in pairs of deletes and inserts, helping visualize on a
line-by-line basis how the content has changed.

This change doesn't require any JavaScript or server-side work.

There are some queries using the modern :has() selector, which Firefox
doesn't fully support. This is just for cleaner border styling, and is
not detrimental. Firefox will have improved styling once this is fully
supported (which, according to bug reports, should be any day now).

Tested on Chrome, Firefox, and Safari with and witout Subgrids.

Tested the diff viewer on both desktop and mobile views.

On mobile, I tested move detection, commenting (single- and multi-line),
file downloads, move flags, expansion/collapsing, and interdiffs, with
all kinds of mixes of inserts/deletes/replaces.

Summary ID
Add a mobile view for the diff viewer.
This introduces a long-awaited mobile view for the diff viewer. This is a pure-CSS solution for viewing existing diffs in a mobile, unified diff display, without the need for a re-render of a diff. All diff functionality works as normally. Users can browse the diff, expand, collapse, download files, make multi-line comments, view/navigate moved line flags, and view interdiffs (though the revision slider has interaction problems on mobile). On mobile, the diff viewer is rendered as a CSS Grid, with three columns: Original line number, modified line number, and line content. The grid's column setup provides ample room for large line numbers (safely up to 6 digits) before a given row's line number column truncates (it will truncate on the left). This is for browsers without support for Subgrids. For those that do support Subgrids, the columns will resize for the number of digits needed (and reserve less space to start). Inserts, deletes, and equals present the same as before, though only one column of line content is shown. Replaces work differently, as they're shown in pairs of deletes and inserts, helping visualize on a line-by-line basis how the content has changed. This change doesn't require any JavaScript or server-side work. While a last-minute addition for Review Board 6, the nature of the change makes this safe to include in this release (by default, the diff viewer is completely busted on mobile, and this is certainly a step up).
52dbd0f08235ffdcc83b86490c4aabfd634734f2

Description From Last Updated

Typo: "styled" -> "styles"

maubinmaubin
david
  1. While this should theoretically be safe and a strict improvement over what we have now, it makes me very nervous to be dropping changes of this size and scope this close to release.

    Given that we want to turn around a 7.0 pretty quickly in order to shift us over to Django 4.2, how would you feel about making the mobile diffviewer stuff be a feature of that release instead?

    1. I've been mixed on this.

      Initially, I wasn't going to advocate for this for 6.0. However, given how smoothly it's worked in all my testing, and how what we already had on mobile was just outright busted and unusable, this is a strict improvement. It'd also pair well with the new review banner, selling a larger review story. It can't regress desktop, so I'm not worried much about risk.

      Those are my pros in favor of landing it in 6.0.

      My concerns are that, yeah, it's a big change and there could be corner cases I didn't hit, which could result in a subpar experience in places. Those would be limited to mobile users. By postponing, perhaps I could also clean up some of the CSS overall, but that wouldn't be a requirement for the feature, just more work that could be done.

      And by delaying it, we could pair this with other diff viewer enhancements. There's an argument to be made there.

      So while I feel okay landing this for 6.0, I also feel okay delaying it to 7.0.

      In either case, given the lack of beta feedback throughout this release, I think the only testing we'd realistically get is once this is in production. Most people aren't going to be using this daily anyway, so are likely to only notice issues if, say, going for a walk and reviewing code. That won't happen in a beta setting.

    2. Okay, let's punt for 6.0, and make it part of a larger diff viewer improvement for 7.

    3. Thanks. I think this also gives us a nice head start on a feature set for 7.0 beyond django/python version bumps. 5 months is going to pass by before we know it, especially with me and Michelle distracted on other things.

  2. 
      
maubin
  1. Looks great :).

  2. Typo: "styled" -> "styles"

  3. 
      
chipx86
Review request changed

Description:

   

This introduces a long-awaited mobile view for the diff viewer. This is

    a pure-CSS solution for viewing existing diffs in a mobile, unified diff
    display, without the need for a re-render of a diff.

   
   

All diff functionality works as normally. Users can browse the diff,

    expand, collapse, download files, make multi-line comments,
    view/navigate moved line flags, and view interdiffs (though the revision
    slider has interaction problems on mobile).

   
   

On mobile, the diff viewer is rendered as a CSS Grid, with three

    columns: Original line number, modified line number, and line content.

   
   

The grid's column setup provides ample room for large line numbers

    (safely up to 6 digits) before a given row's line number column
    truncates (it will truncate on the left). This is for browsers without
    support for Subgrids. For those that do support Subgrids, the columns
    will resize for the number of digits needed (and reserve less space to
    start).

   
   

Inserts, deletes, and equals present the same as before, though only one

    column of line content is shown. Replaces work differently, as they're
    shown in pairs of deletes and inserts, helping visualize on a
    line-by-line basis how the content has changed.

   
~  

This change doesn't require any JavaScript or server-side work. While a

  ~

This change doesn't require any JavaScript or server-side work.

-   last-minute addition for Review Board 6, the nature of the change makes
-   this safe to include in this release (by default, the diff viewer is
-   completely busted on mobile, and this is certainly a step up).

   
   

There are some queries using the modern :has() selector, which Firefox

    doesn't fully support. This is just for cleaner border styling, and is
    not detrimental. Firefox will have improved styling once this is fully
    supported (which, according to bug reports, should be any day now).

Branch:

-release-6.x
+master
Loading...