Make the file index dock into the unified banner.

Review Request #12941 — Created April 5, 2023 and submitted

Information

Review Board
release-6.x

Reviewers

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 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
Description From Last Updated

Could add some documentation for this component while you're here.

maubinmaubin

Should documentation this new component and its child.

maubinmaubin

I think this is supposed to say "The top of the diff entry..."

maubinmaubin

Missing docs for the interface and members.

chipx86chipx86

We should probably also remove/clear all the other jQuery and map state, so we don't retain any references.

chipx86chipx86

This can be return super.remove();

chipx86chipx86

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

chipx86chipx86

Can we do one type per line?

chipx86chipx86

Can we pull ou this.#$items before the look, so we don't have to re-fetch every iteration?

chipx86chipx86

Same comment about these variables.

chipx86chipx86

Is the Number just for typing? Do we not get this from .entries()?

chipx86chipx86

And these.

chipx86chipx86

Would it be worth asserting the result before returning?

chipx86chipx86

Maybe this is a good time to switch to our modern spinner class? djblets-o-spinner.

chipx86chipx86
david
maubin
  1. 
      
  2. Show all issues

    Could add some documentation for this component while you're here.

  3. Show all issues

    Should documentation this new component and its child.

  4. Show all issues

    I think this is supposed to say "The top of the diff entry..."

  5. 
      
david
chipx86
  1. 
      
  2. Show all issues

    Missing docs for the interface and members.

  3. Show all issues

    We should probably also remove/clear all the other jQuery and map state, so we don't retain any references.

  4. Show all issues

    This can be return super.remove();

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

  6. Show all issues

    Can we do one type per line?

  7. Show all issues

    Can we pull ou this.#$items before the look, so we don't have to re-fetch every iteration?

  8. reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 3)
     
     
     
     
     
     
     
    Show all issues

    Same comment about these variables.

  9. Show all issues

    And these.

  10. Show all issues

    Would it be worth asserting the result before returning?

  11. Show all issues

    Maybe this is a good time to switch to our modern spinner class? djblets-o-spinner.

  12. 
      
david
chipx86
  1. 
      
  2. Show all issues

    Is the Number just for typing? Do we not get this from .entries()?

    1. This was left over from when these were objects instead of maps.

  3. 
      
david
maubin
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-6.x (05dad17)