Port PageView to spina.

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

Information

Review Board
release-6.x

Reviewers

This change ports the base PageView class to spina and TypeScript. As
part of this, the code has been moved into the common subdirectory to
match the bundle that it's a part of.

The one change that was required outside of this class was to update the
DiffViewerPageModel to use preinitialize instead of constructor.
The use of the reserved name constructor has bitten us in the past,
and wasn't working anymore now that the base class was an ES6 class.

  • Ran js-tests.
  • Verified that page views still rendered correctly.
Summary ID
Port PageView to spina.
This change ports the base PageView class to spina and TypeScript. As part of this, the code has been moved into the `common` subdirectory to match the bundle that it's a part of. The one change that was required outside of this class was to update the `DiffViewerPageModel` to use `preinitialize` instead of `constructor`. The use of the reserved name `constructor` has bitten us in the past, and wasn't working anymore now that the base class was an ES6 class. Testing Done: - Ran js-tests. - Verified that page views still rendered correctly.
bf63aeed7cb7febfec71b026b87c46c6acc793c9
Description From Last Updated

Since this was accessible on the prototype, but isn't really an instance variable, I wonder if we should make this …

chipx86chipx86

Can we work to document all instance variables going forward?

chipx86chipx86

We can return super.remove() instead.

chipx86chipx86

Since this has multiple types involved, let's move actionId to its own line, so the :'s don't all run together.

chipx86chipx86

Are you able to just import 'jasmine-core' and get the same results? I've resolved linting locally with just an import …

chipx86chipx86
chipx86
  1. 
      
  2. Show all issues

    Since this was accessible on the prototype, but isn't really an instance variable, I wonder if we should make this static and include it in @spina({prototypeAttrs: [...]})?

    1. Will that work with overriding in inherited classes?

    2. OK, just tested and it does work as long as the subclass also uses @spina({prototypeAttrs: [...]})

    3. Oh, that's supposed to inherit. Hmm... That'd be a bug in Spina if it doesn't.

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

    Can we work to document all instance variables going forward?

  4. Show all issues

    We can return super.remove() instead.

  5. Show all issues

    Since this has multiple types involved, let's move actionId to its own line, so the :'s don't all run together.

  6. reviewboard/static/rb/js/common/views/tests/pageViewTests.ts (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Are you able to just import 'jasmine-core' and get the same results? I've resolved linting locally with just an import 'jasmine', and wasn't sure if it'd work the same way.

    1. No, that doesn't seem to work.

  7. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-6.x (27a193d)