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)