Port RB.MenuButtonView to TypeScript and spina, remove SplitButtonView.

Review Request #12824 — Created Feb. 3, 2023 and submitted

Information

Review Board
release-6.x

Reviewers

This change ports the MenuButtonView UI element over to our new
TypeScript/spina framework.

The MenuButtonView had mostly replaced the old SplitButtonView,
which had been marked as deprecated since 4.0 (and which had become a
simple wrapper). As part of this change, I've moved the couple old uses
of SplitButtonView over to use MenuButtonView directly.

  • Verified "Publish" button functionality on the review dialog and draft
    review banner.
  • Ran js-tests.
Summary ID
Port RB.MenuButtonView to TypeScript and spina, remove SplitButtonView.
This change ports the MenuButtonView UI element over to our new TypeScript/spina framework. The `MenuButtonView` had mostly replaced the old `SplitButtonView`, which had been marked as deprecated since 4.0 (and which had become a simple wrapper). As part of this change, I've moved the couple old uses of SplitButtonView over to use MenuButtonView directly. Testing Done: - Verified "Publish" button functionality on the review dialog and draft review banner. - Ran js-tests.
ed1a24257f057c3a4b40644f083400253fd51ab9
Description From Last Updated

Can we make sure we're using one-line summaries for these? These will all eventually go through the same Napoleon doc …

chipx86chipx86

One-line/sentence summary here too. And below.

chipx86chipx86

Blank line between these. Makes it easier to document the instance variables when public. And actually, we should probably stick …

chipx86chipx86

Wonder if we should start making these static. I can't remember if there are currently any issues using that.

chipx86chipx86

I don't think we need to call the parent when subclassing BaseView/View.

chipx86chipx86

No need to call the parent here. It's explicitly empty.

chipx86chipx86

We should use ... as Element instead of <Element>.... This is the recommended syntax in TypeScript going forward (designed to …

chipx86chipx86

Can we sort this?

chipx86chipx86

This shouldn't be indented.

chipx86chipx86

We should also include Version Added here.

chipx86chipx86

We can use the _ template literal stuff for these gettext calls. (_ makes ESLint grumble a bit, but I …

chipx86chipx86

Leftover debugging.

chipx86chipx86

Leftover debugging.

chipx86chipx86

Can we put this in alphabetical order?

chipx86chipx86

These should be combined, updated for bullet points.

chipx86chipx86
chipx86
  1. 
      
  2. Show all issues

    Can we make sure we're using one-line summaries for these? These will all eventually go through the same Napoleon doc handling that the Python code goes through, so it'll need to conform to that.

  3. Show all issues

    One-line/sentence summary here too.

    And below.

  4. Show all issues

    Blank line between these. Makes it easier to document the instance variables when public.

    And actually, we should probably stick to the usual ordering of public and then private.

  5. Show all issues

    Wonder if we should start making these static. I can't remember if there are currently any issues using that.

    1. We can do private or we can do static, but static private can't be used with decorators, for some reason. Given the choice, I think I prefer private.

  6. Show all issues

    I don't think we need to call the parent when subclassing BaseView/View.

  7. Show all issues

    No need to call the parent here. It's explicitly empty.

  8. Show all issues

    We should use ... as Element instead of <Element>.... This is the recommended syntax in TypeScript going forward (designed to avoid issues with JSX).

    Here and anywhere else we may be doing it.

  9. reviewboard/static/rb/js/ui/views/menuButtonView.ts (Diff revision 1)
     
     
     
     
     
    Show all issues

    Can we sort this?

    1. Yes, but I'm also going to switch back to if statements as mentioned in another change.

  10. Show all issues

    This shouldn't be indented.

  11. Show all issues

    We should also include Version Added here.

  12. Show all issues

    We can use the _ template literal stuff for these gettext calls.

    (_ makes ESLint grumble a bit, but I plan to address that.)

  13. Show all issues

    Leftover debugging.

  14. Show all issues

    Leftover debugging.

  15. 
      
david
chipx86
  1. 
      
  2. reviewboard/static/rb/js/ui/index.ts (Diff revision 2)
     
     
     
    Show all issues

    Can we put this in alphabetical order?

  3. reviewboard/static/rb/js/ui/views/menuView.es6.js (Diff revision 2)
     
     
     
     
     
     
     
     
    Show all issues

    These should be combined, updated for bullet points.

  4. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-6.x (2d165c6)