[WIP] Added a floating banner to the diff view

Review Request #8746 — Created Feb. 14, 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 on the page. On this banner, I will further add a
collapsed list of the diff files, showing only the the file that the user is currently
looking at. The user will be able to expand this list to navigate to the other files.

Screenshots and demo video: Notion

I have tested the floating banner on the dev server on review requests with multiple
diff files.

I have started writing jasmine tests for the floating banner. This is still a WIP.
The banner has been manually tested (including responsiveness) on Chrome, Safari, and
Firefox. Click here to see GIFs of the manual tests (running on diff revision 13).

Further testing on other browsers are required.

Description From Last Updated

I think this would be more readable if this else was on a new line.

DA DanielPBak

Same here.

DA DanielPBak

remove blank line

RK rkdhatt

Alphabetize

brenniebrennie

Doc comments begin with /**.

brenniebrennie

Doc comments begin with /** in JS

brenniebrennie

How about viewType

brenniebrennie

This can always run, since it is just a call to bind. It doesn't hurt. Alternatively we can add this …

brenniebrennie

Instead of using each, we should do a for loop. That way we can break out after finding the element …

brenniebrennie

This isnt a jQuery. To get a jQuery (and use the .show() method i suggested) use this._$items.eq(index).

brenniebrennie

you can do $bannerTableRow.show() here.

brenniebrennie

This should be a class, not an ID.

brenniebrennie

border has a shorthand property as following: border: border-width border-style border-color or you can define border-width as following: border-width: top …

AN anni_cao

Why do you need break at the end of if statement? It will jump out of the loop anyway, right?

AN anni_cao
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffReviewableCollection.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffReviewableCollection.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
MO
MO
DA
  1. 
      
  2. I think this would be more readable if this else was on a new line.

    1. I totally agree with you. However, I am following the same coding style / conventions
      that are used elsewhere in RB's codebase. The rest of RB's Javascript codebase uses
      else statements on the same line as the curly bracket.

    1. See reply above.

  3. 
      
MO
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
MO
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
MO
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
MO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
RK
  1. 
      
  2. remove blank line

  3. 
      
MO
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
MO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
MO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
MO
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
brennie
  1. 
      
  2. Doc comments begin with /**.

  3. Doc comments begin with /** in JS

  4. This can always run, since it is just a call to bind. It doesn't hurt.

    Alternatively we can add this specific behaviour in a subclass. (That is what I would prefer here, but you may want to ask another mentor's opinion.)

    1. I suggested using a subclass to Christian earlier, and he mentioned that we didn't need a subclass in this case since the class only has two modes (page-view and banner-view). He said we should use a subclass if it had more than two modes (if I recall correctly).

  5. reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Instead of using each, we should do a for loop. That way we can break out after finding the element to display (because if its the first one of twenty, there is no need to consider the next 19). It also lets us use a more correct way of determing if something is in view:

    windowBottom = windowTop + $(window).height();
    
    for (i = 0; i < this.collection.size(); i++) {
        /* variable assignments */
    
        if (filePositionTop <= windowTop && filePositionBottom >= windowTop ||
            filePositionTop >= windowTop && filePositionTop <= windowTop + $(window).height()) {
            /* in view, show it */
            break;
         }
    }
    

    The first condition checks if the file is above or starting at the top of the window and the second checks that the top of the file is within the window viewport.

    However, you will also have to hide all other elements which you can do with this._$items.hide() before the loop.

    1. I am noticing a couple of issues with this implementation. The break statement creates issues when scrolling up in the view; the diff files are never removed, so it ends up appending each diff file as it enters the screen. It never gets to the hide() function for the diff file below it because it breaks out of the loop.

      Therefore, I tried removing the break statement. In this case, each file inside the view is shown. For example, if four small diff files are currently inside the window, then all four appears in the banner as default.

      I came up with a working solution for only displaying one row as default:

      for (var i = 0; i < this.collection.size(); i++) {
          diffFileId = '#file' + this.collection.models[i].attributes.id;
          diffFileContainer = $(diffFileId).parent().parent();   //get the "diff-container"
          containerTop = diffFileContainer.offset().top;
          containerBottom = containerTop + diffFileContainer.outerHeight(true);
          $bannerTableRow = this._$items.eq(i);
          /**
           * Show the table row only if it intersects with the top of the window.
           */
           if (windowTop >= containerTop && windowTop < containerBottom) {
               $bannerTableRow.show();
               $bannerTableRow.attr('id', 'banner-active-file');
           } else {
               $bannerTableRow.hide();
               if ($bannerTableRow.is('#banner-active-file')) {
                   $bannerTableRow.removeAttr('id');
               }
          }
      }
      

      It is very similar to the one I previously had, and the major difference is that I'm getting the diff-container rather than the table inside the diff-container. This solution also takes the margin size into account between each diff-container (I tested it by changing the margin-bottom value in the inspector).

      Let me know what you think.

  6. This isnt a jQuery. To get a jQuery (and use the .show() method i suggested) use this._$items.eq(index).

  7. you can do $bannerTableRow.show() here.

  8. 
      
MO
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/views/floatingBannerView.js
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
        reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
brennie
  1. 
      
  2. This should be a class, not an ID.

  3. 
      
MO
MO
MO
MO
AN
  1. 
      
  2. Why do you need break at the end of if statement? It will jump out of the loop anyway, right?

    1. The break statement optimizes the number of computations we have to do in the loop.
      For example, if the size of the list we are looping through is 20, and we find the
      file that we want after 3 iterations, the break statement skips the next 17 iterations.

  3. 
      
AN
  1. 
      
  2. border has a shorthand property as following:
    border: border-width border-style border-color

    or you can define border-width as following:
    border-width: top right bottom left

    In your case, looks like they are all 0, you can do
    border-width: 0

    1. Thanks for pointing this out, Anni! I fixed it by doing this:

      border: 0 @review-request-border-color;
      

      I also realized I had hard-coded the colors instead of using RB's .less color definitions.

  3. 
      
MO
MO
MO
MO
david
Review request changed

Status: Discarded

Loading...