Make the file index dock into the unified banner.
Review Request #12941 — Created April 5, 2023 and submitted
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.
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.
Summary | |
---|---|
Description | From | Last Updated |
---|---|---|
Could add some documentation for this component while you're here. |
![]() |
|
Should documentation this new component and its child. |
![]() |
|
I think this is supposed to say "The top of the diff entry..." |
![]() |
|
Missing docs for the interface and members. |
|
|
We should probably also remove/clear all the other jQuery and map state, so we don't retain any references. |
|
|
This can be return super.remove(); |
|
|
This stays away even after remove(). We should listen to it with an ID incorporating this.cid and then stop listening … |
|
|
Can we do one type per line? |
|
|
Can we pull ou this.#$items before the look, so we don't have to re-fetch every iteration? |
|
|
Same comment about these variables. |
|
|
Is the Number just for typing? Do we not get this from .entries()? |
|
|
And these. |
|
|
Would it be worth asserting the result before returning? |
|
|
Maybe this is a good time to switch to our modern spinner class? djblets-o-spinner. |
|
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: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+754 -184) |
Checks run (2 succeeded)

-
-
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 2) Could add some documentation for this component while you're here.
-
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 2) Should documentation this new component and its child.
-
reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 2) I think this is supposed to say "The top of the diff entry..."
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+786 -184) |
Checks run (2 succeeded)
-
-
reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 3) Missing docs for the interface and members.
-
reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 3) 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) This can be
return super.remove();
-
reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 3) This stays away even after
remove()
. We should listen to it with an ID incorporatingthis.cid
and then stop listening inremove()
. -
reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 3) Can we do one type per line?
-
reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 3) 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) Same comment about these variables.
-
-
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) Would it be worth asserting the result before returning?
-
reviewboard/templates/diffviewer/view_diff.html (Diff revision 3) Maybe this is a good time to switch to our modern spinner class?
djblets-o-spinner
.
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+842 -184) |
Checks run (2 succeeded)
-
-
reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revisions 3 - 4) Is the
Number
just for typing? Do we not get this from.entries()
?
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+842 -184) |