• 
      

    Create RB.Admin.PageView and update existing admin views to extend it

    Review Request #10978 — Created March 27, 2020 and submitted

    Information

    Review Board
    master
    4a1431f...

    Reviewers

    This review request creates RB.Admin.PageView, a new base page view that is
    specific to admin views. Existing views in reviewboard/static/rb/js/admin/views
    that don't extend from a specific view are updated to extend RB.Admin.PageView.
    They also call the parent view's initialize and renderPage as needed. This
    change will help with the implementation of the admin setup banner, which needs
    to be displayed on all admin pages.

    Existing unit tests all pass and a manual inspection of admin pages shows that
    pages still render correctly.

    Description From Last Updated

    Similar to David's comment, this isn't a page view, so it shouldn't inherit from RB.Admin.PageView. You can completely revert this …

    chipx86chipx86

    I don't think this change is correct. There should be only one PageView object for the entire page, but this …

    daviddavid

    There's an extra line here.

    daviddavid

    Unlike render(), renderPage() doesn't return anything. So the function would be empty, just like RB.PageView's. As such, it's probably best …

    chipx86chipx86

    This is still incorrectly inheriting from PageView. We should revert all the changes in this file.

    daviddavid
    david
    1. 
        
    2. Show all issues

      I don't think this change is correct. There should be only one PageView object for the entire page, but this is a sub-view that's instantiated (multiple times) by ChangeFormPageView.

    3. Show all issues

      There's an extra line here.

    4. 
        
    chipx86
    1. 
        
    2. Show all issues

      Similar to David's comment, this isn't a page view, so it shouldn't inherit from RB.Admin.PageView. You can completely revert this file.

    3. reviewboard/static/rb/js/admin/views/pageView.es6.js (Diff revision 1)
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      Unlike render(), renderPage() doesn't return anything. So the function would be empty, just like RB.PageView's.

      As such, it's probably best we just leave this out entirely in this change. There's no value to having renderPage() be here, since a parent class calling RB.Admin.PageView.protoype.renderPage() will end up calling the base RB.PageView.prototype.renderPage().

      This function can be defined and implemented when a future change needs to do so.

      So really, the code for this view should probably just be:

      RB.Admin.PageView = RB.PageView.extend({
      });
      
    4. 
        
    hxqlin
    david
    1. 
        
    2. Show all issues

      This is still incorrectly inheriting from PageView. We should revert all the changes in this file.

      1. Woops, I misread and thought both your and Christian's comment was referring to the same file.

    3. 
        
    hxqlin
    chipx86
    1. Ship It!
    2. 
        
    hxqlin
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-4.0.x (0ebfa48)