[WIP] Added a floating banner to the diff view

Review Request #8714 — Created Feb. 4, 2017 and discarded

Information

Review Board
master

Reviewers

We currently have to scroll all the way to the top if we want to be navigated to a
specific diff file.

I added a floating banner to the diff view that sticks to the top of the page when
the user has scrolled down. I added text to the banner: "Navigate: ", as well as
a dropdown button. This button is a list of all diff file names. The current
state displays the first diff filename on default, and each diff filename
when hovering over the arrow. The default filename is duplicated at the
moment. I also added scrolling functionality when the button gets clicked.
This will take the user's view to the anchor tag of the selected diff file.

Screenshots of the current state of my project:
https://www.notion.so/Week-2-Feb-4th-6eb70ca4cd8e4c4583bbf4d439533e31

I have tested the floating banner on the testing server using dummy diff files
created by RB Tools.

Description From Last Updated

Correct me if I'm wrong, but: I don't understand why you're attaching the diff-banner to the template meant for each …

szhangszhang
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/static/rb/css/pages/diffviewer.less
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/static/rb/css/pages/diffviewer.less
    
    
  2. 
      
MO
szhang
  1. 
      
  2. reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1)
     
     
     
     
     
     
     

    Correct me if I'm wrong, but: I don't understand why you're attaching the diff-banner to the template meant for each file's diff in the diff page.

    On line 266, $diffs is assigned to the empty div with the id #diffs:

    $diffs = $('#diffs').empty();
    

    Then on line 270, a function begins that executes for each file:

    files.each(function(file) {
    

    In this function, the line adding the template is here (line 275):

    $diffs.append(this._fileEntryTemplate(file.attributes));
    

    So it appears that the code adds the template multiple times to the div with the id #diffs.

    I don't think you want to make the banner appear multiple times, just once. Perhaps there is a better place to add the banner, like reviewboard/templates/diffviewer/view_diff.html?

    1. Thanks a lot for pointing out this bug / bad practice, Simon!

      I didn't get the chance to test the banner on review
      requests containing multiple diff files, and I can
      only imagine what it would look like if I had 10+
      diff files in the diff view.

      Your explanation makes total sense. My intentions were not
      to add it once for each diff file, so this was an awesome
      catch. I removed the divs from the diffViewerPageView and
      added them to view_diff.html.

      The banner only gets added once now.

    2. Let me correct myself: My next [WIP] review request will include the update!

  3. 
      
MO
Review request changed

Status: Discarded

Change Summary:

multiple review requests for the same changes.

Loading...