Summary

Make the file index dock into the unified banner.

Review Request #12941 — Created April 5, 2023 and submitted April 25, 2023, 11:42 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-6.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change makes it so the file list docks into the unified banner once
the user scrolls past it. The view there is filtered to show only the
entries corresponding to any diff file blocks visible in the current
viewport. There's a 50px buffer area at the top and bottom in which we
calculate a partial vertical offset and height so it looks like the file
list is scrolling along with the page.

This does not yet allow users to pop open the entire file list in order
to quickly scroll to a specific file, but that's coming in a future
change.

Testing Done

Spent a ton of time scrolling around a large diff and verifying that the
docked file list looked correct. Paid particular attention to the
boundary at which we convert the file list from undocked -> docked and
vice-versa, and the boundaries where files were scrolling on or off the
page at both the top and bottom.

Commits

Summary	ID
Make the file index dock into the unified banner. One common request we've had over the years is to be able to know what file is currently being viewed when scrolling through the diffviewer. It's easy to lose track of the current file in the middle of a long diff. This change makes it so the file list docks into the unified banner once the user scrolls past it. The view there is filtered to show only the entries corresponding to any diff file blocks visible in the current viewport. There's a 50px buffer area at the top and bottom in which we calculate a partial vertical offset and height so it looks like the file list is scrolling along with the page. This does not yet allow users to pop open the entire file list in order to quickly scroll to a specific file, but that's coming in a future change. Testing Done: Spent a ton of time scrolling around a large diff and verifying that the docked file list looked correct. Paid particular attention to the boundary at which we convert the file list from undocked -> docked and vice-versa, and the boundaries where files were scrolling on or off the page at both the top and bottom.	0cb201a77b7aa1f294e9a757b254e4ad38205f98

Summary

Make the file index dock into the unified banner.

One common request we've had over the years is to be able to know what file is currently being viewed when scrolling through the diffviewer. It's easy to lose track of the current file in the middle of a long diff. This change makes it so the file list docks into the unified banner once the user scrolls past it. The view there is filtered to show only the entries corresponding to any diff file blocks visible in the current viewport. There's a 50px buffer area at the top and bottom in which we calculate a partial vertical offset and height so it looks like the file list is scrolling along with the page. This does not yet allow users to pop open the entire file list in order to quickly scroll to a specific file, but that's coming in a future change. Testing Done: Spent a ton of time scrolling around a large diff and verifying that the docked file list looked correct. Paid particular attention to the boundary at which we convert the file list from undocked -> docked and vice-versa, and the boundaries where files were scrolling on or off the page at both the top and bottom.

0cb201a77b7aa1f294e9a757b254e4ad38205f98

Issues

Description	From	Last Updated
Could add some documentation for this component while you're here.	maubin	April 17, 2023, 10:08 a.m.
Should documentation this new component and its child.	maubin	April 17, 2023, 10:08 a.m.
I think this is supposed to say "The top of the diff entry..."	maubin	April 17, 2023, 10:05 a.m.
Missing docs for the interface and members.	chipx86	April 18, 2023, 11:51 a.m.
We should probably also remove/clear all the other jQuery and map state, so we don't retain any references.	chipx86	April 18, 2023, 11:53 a.m.
This can be return super.remove();	chipx86	April 18, 2023, 11:53 a.m.
This stays away even after remove(). We should listen to it with an ID incorporating this.cid and then stop listening …	chipx86	April 18, 2023, 11:55 a.m.
Can we do one type per line?	chipx86	April 18, 2023, 11:55 a.m.
Can we pull ou this.#$items before the look, so we don't have to re-fetch every iteration?	chipx86	April 18, 2023, 11:56 a.m.
Same comment about these variables.	chipx86	April 18, 2023, 11:57 a.m.
Is the Number just for typing? Do we not get this from .entries()?	chipx86	April 19, 2023, 3:45 p.m.
And these.	chipx86	April 18, 2023, 12:10 p.m.
Would it be worth asserting the result before returning?	chipx86	April 18, 2023, 12:10 p.m.
Maybe this is a good time to switch to our modern spinner class? djblets-o-spinner.	chipx86	April 18, 2023, 12:15 p.m.

flake8 passed.

JSHint passed.

Change Summary:


Fixed ugly display when file entries were short (such as only one chunk).
Added some bullet-proofing to properly handle changing diff/commit revisions and trying to scroll to anchors before everything is loaded.

Commits:

	Summary	ID
	Make the file index dock into the unified banner. One common request we've had over the years is to be able to know what file is currently being viewed when scrolling through the diffviewer. It's easy to lose track of the current file in the middle of a long diff. This change makes it so the file list docks into the unified banner once the user scrolls past it. The view there is filtered to show only the entries corresponding to any diff file blocks visible in the current viewport. There's a 50px buffer area at the top and bottom in which we calculate a partial vertical offset and height so it looks like the file list is scrolling along with the page. This does not yet allow users to pop open the entire file list in order to quickly scroll to a specific file, but that's coming in a future change. Testing Done: Spent a ton of time scrolling around a large diff and verifying that the docked file list looked correct. Paid particular attention to the boundary at which we convert the file list from undocked -> docked and vice-versa, and the boundaries where files were scrolling on or off the page at both the top and bottom.	de8b86e30711ee03e9590972d26e8eae35c59063
	Make the file index dock into the unified banner. One common request we've had over the years is to be able to know what file is currently being viewed when scrolling through the diffviewer. It's easy to lose track of the current file in the middle of a long diff. This change makes it so the file list docks into the unified banner once the user scrolls past it. The view there is filtered to show only the entries corresponding to any diff file blocks visible in the current viewport. There's a 50px buffer area at the top and bottom in which we calculate a partial vertical offset and height so it looks like the file list is scrolling along with the page. This does not yet allow users to pop open the entire file list in order to quickly scroll to a specific file, but that's coming in a future change. Testing Done: Spent a ton of time scrolling around a large diff and verifying that the docked file list looked correct. Paid particular attention to the boundary at which we convert the file list from undocked -> docked and vice-versa, and the boundaries where files were scrolling on or off the page at both the top and bottom.	e944cb5b737bdb68d212a9ac14ef1dab90afd51b

Diff:

Revision 2 (+754 -184)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts
	reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts
	reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts
	reviewboard/templates/diffviewer/view_diff.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 2)
The issue has been resolved. Show all issues
```
Could add some documentation for this component while you're here.
```
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 2)
The issue has been resolved. Show all issues
```
Should documentation this new component and its child.
```
reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 2)
The issue has been resolved. Show all issues
```
I think this is supposed to say "The top of the diff entry..."
```

Commits:

	Summary	ID
	Make the file index dock into the unified banner. One common request we've had over the years is to be able to know what file is currently being viewed when scrolling through the diffviewer. It's easy to lose track of the current file in the middle of a long diff. This change makes it so the file list docks into the unified banner once the user scrolls past it. The view there is filtered to show only the entries corresponding to any diff file blocks visible in the current viewport. There's a 50px buffer area at the top and bottom in which we calculate a partial vertical offset and height so it looks like the file list is scrolling along with the page. This does not yet allow users to pop open the entire file list in order to quickly scroll to a specific file, but that's coming in a future change. Testing Done: Spent a ton of time scrolling around a large diff and verifying that the docked file list looked correct. Paid particular attention to the boundary at which we convert the file list from undocked -> docked and vice-versa, and the boundaries where files were scrolling on or off the page at both the top and bottom.	e944cb5b737bdb68d212a9ac14ef1dab90afd51b
	Make the file index dock into the unified banner. One common request we've had over the years is to be able to know what file is currently being viewed when scrolling through the diffviewer. It's easy to lose track of the current file in the middle of a long diff. This change makes it so the file list docks into the unified banner once the user scrolls past it. The view there is filtered to show only the entries corresponding to any diff file blocks visible in the current viewport. There's a 50px buffer area at the top and bottom in which we calculate a partial vertical offset and height so it looks like the file list is scrolling along with the page. This does not yet allow users to pop open the entire file list in order to quickly scroll to a specific file, but that's coming in a future change. Testing Done: Spent a ton of time scrolling around a large diff and verifying that the docked file list looked correct. Paid particular attention to the boundary at which we convert the file list from undocked -> docked and vice-versa, and the boundaries where files were scrolling on or off the page at both the top and bottom.	194026da2b7d0435aee35a7c1839ff9fa1b27ac9

Diff:

Revision 3 (+786 -184)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts
	reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts
	reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts
	reviewboard/templates/diffviewer/view_diff.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 3)

The issue has been resolved. Show all issues

Missing docs for the interface and members.

reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 3)
The issue has been resolved. Show all issues
```
We should probably also remove/clear all the other jQuery and map state, so we don't retain any references.
```
reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 3)
The issue has been resolved. Show all issues
```
This can be return super.remove();
```

reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 3)

The issue has been resolved. Show all issues

This stays away even after remove(). We should listen to it with an ID incorporating this.cid and then stop listening in remove().

reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we do one type per line?
```
reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we pull ou this.#$items before the look, so we don't have to re-fetch every iteration?
```

reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 3)

The issue has been resolved. Show all issues

Same comment about these variables.

reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 3)
The issue has been resolved. Show all issues
```
And these.
```
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3)
The issue has been resolved. Show all issues
```
Would it be worth asserting the result before returning?
```
reviewboard/templates/diffviewer/view_diff.html (Diff revision 3)
The issue has been resolved. Show all issues
```
Maybe this is a good time to switch to our modern spinner class? djblets-o-spinner.
```

Commits:

	Summary	ID
	Make the file index dock into the unified banner. One common request we've had over the years is to be able to know what file is currently being viewed when scrolling through the diffviewer. It's easy to lose track of the current file in the middle of a long diff. This change makes it so the file list docks into the unified banner once the user scrolls past it. The view there is filtered to show only the entries corresponding to any diff file blocks visible in the current viewport. There's a 50px buffer area at the top and bottom in which we calculate a partial vertical offset and height so it looks like the file list is scrolling along with the page. This does not yet allow users to pop open the entire file list in order to quickly scroll to a specific file, but that's coming in a future change. Testing Done: Spent a ton of time scrolling around a large diff and verifying that the docked file list looked correct. Paid particular attention to the boundary at which we convert the file list from undocked -> docked and vice-versa, and the boundaries where files were scrolling on or off the page at both the top and bottom.	194026da2b7d0435aee35a7c1839ff9fa1b27ac9
	Make the file index dock into the unified banner. One common request we've had over the years is to be able to know what file is currently being viewed when scrolling through the diffviewer. It's easy to lose track of the current file in the middle of a long diff. This change makes it so the file list docks into the unified banner once the user scrolls past it. The view there is filtered to show only the entries corresponding to any diff file blocks visible in the current viewport. There's a 50px buffer area at the top and bottom in which we calculate a partial vertical offset and height so it looks like the file list is scrolling along with the page. This does not yet allow users to pop open the entire file list in order to quickly scroll to a specific file, but that's coming in a future change. Testing Done: Spent a ton of time scrolling around a large diff and verifying that the docked file list looked correct. Paid particular attention to the boundary at which we convert the file list from undocked -> docked and vice-versa, and the boundaries where files were scrolling on or off the page at both the top and bottom.	0f29d703849787b4d1007a7dce67367c405864aa

Diff:

Revision 4 (+842 -184)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts
	reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts
	reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts
	reviewboard/templates/diffviewer/view_diff.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revisions 3 - 4)
The issue has been resolved. Show all issues
```
Is the Number just for typing? Do we not get this from .entries()?
```
1. david April 19, 2023, 3:54 p.m.
  This was left over from when these were objects instead of maps.

Commits:

	Summary	ID
	Make the file index dock into the unified banner. One common request we've had over the years is to be able to know what file is currently being viewed when scrolling through the diffviewer. It's easy to lose track of the current file in the middle of a long diff. This change makes it so the file list docks into the unified banner once the user scrolls past it. The view there is filtered to show only the entries corresponding to any diff file blocks visible in the current viewport. There's a 50px buffer area at the top and bottom in which we calculate a partial vertical offset and height so it looks like the file list is scrolling along with the page. This does not yet allow users to pop open the entire file list in order to quickly scroll to a specific file, but that's coming in a future change. Testing Done: Spent a ton of time scrolling around a large diff and verifying that the docked file list looked correct. Paid particular attention to the boundary at which we convert the file list from undocked -> docked and vice-versa, and the boundaries where files were scrolling on or off the page at both the top and bottom.	0f29d703849787b4d1007a7dce67367c405864aa
	Make the file index dock into the unified banner. One common request we've had over the years is to be able to know what file is currently being viewed when scrolling through the diffviewer. It's easy to lose track of the current file in the middle of a long diff. This change makes it so the file list docks into the unified banner once the user scrolls past it. The view there is filtered to show only the entries corresponding to any diff file blocks visible in the current viewport. There's a 50px buffer area at the top and bottom in which we calculate a partial vertical offset and height so it looks like the file list is scrolling along with the page. This does not yet allow users to pop open the entire file list in order to quickly scroll to a specific file, but that's coming in a future change. Testing Done: Spent a ton of time scrolling around a large diff and verifying that the docked file list looked correct. Paid particular attention to the boundary at which we convert the file list from undocked -> docked and vice-versa, and the boundaries where files were scrolling on or off the page at both the top and bottom.	0cb201a77b7aa1f294e9a757b254e4ad38205f98

Diff:

Revision 5 (+842 -184)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts
	reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts
	reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts
	reviewboard/templates/diffviewer/view_diff.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-6.x (05dad17)