Summary

Add a mobile view for the diff viewer.

Review Request #13312 — Created Oct. 6, 2023 and submitted April 17, 2024, 7:38 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

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.

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

Testing Done

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.

Commits

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

Summary

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

6fd7ee542ab4cd7226b848459c75f19bccefc5a0

Files

Issues

Description	From	Last Updated
Typo: "styled" -> "styles"	maubin	April 1, 2024, 7:05 p.m.

flake8 passed.

JSHint passed.

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?

chipx86 Oct. 6, 2023, 3:55 p.m.

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.

chipx86 Oct. 6, 2023, 4:14 p.m.

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

david Oct. 6, 2023, 4:21 p.m.

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.

Ship it!

```
Looks great :).
```
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 1)
The issue has been resolved. Show all issues
```
Typo: "styled" -> "styles"
```

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

Change Summary:

Fixed a typo in docs.

Commits:

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

Branch:

master

release-7.x

Diff:

Revision 2 (+1028 -54)

Show changes

reviewboard/static/rb/css/pages/diffviewer.less

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.x (320a4cd)

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