Summary

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

Review Request #12824 — Created Feb. 3, 2023 and submitted Feb. 27, 2023, 8:28 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-6.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Commits

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

Summary

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

Issues

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 …	chipx86	Feb. 3, 2023, 9:51 p.m.
One-line/sentence summary here too. And below.	chipx86	Feb. 3, 2023, 9:51 p.m.
Blank line between these. Makes it easier to document the instance variables when public. And actually, we should probably stick …	chipx86	Feb. 3, 2023, 9:51 p.m.
Wonder if we should start making these static. I can't remember if there are currently any issues using that.	chipx86	Feb. 3, 2023, 9:52 p.m.
I don't think we need to call the parent when subclassing BaseView/View.	chipx86	Feb. 3, 2023, 9:53 p.m.
No need to call the parent here. It's explicitly empty.	chipx86	Feb. 3, 2023, 9:53 p.m.
We should use ... as Element instead of <Element>.... This is the recommended syntax in TypeScript going forward (designed to …	chipx86	Feb. 3, 2023, 9:54 p.m.
Can we sort this?	chipx86	Feb. 3, 2023, 9:54 p.m.
This shouldn't be indented.	chipx86	Feb. 3, 2023, 9:55 p.m.
We should also include Version Added here.	chipx86	Feb. 3, 2023, 9:55 p.m.
We can use the _ template literal stuff for these gettext calls. (_ makes ESLint grumble a bit, but I …	chipx86	Feb. 3, 2023, 9:56 p.m.
Leftover debugging.	chipx86	Feb. 3, 2023, 9:56 p.m.
Leftover debugging.	chipx86	Feb. 3, 2023, 9:57 p.m.
Can we put this in alphabetical order?	chipx86	Feb. 22, 2023, 8:19 a.m.
These should be combined, updated for bullet points.	chipx86	Feb. 22, 2023, 8:20 a.m.

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/ui/views/menuButtonView.ts (Diff revision 1)

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.

reviewboard/static/rb/js/ui/views/menuButtonView.ts (Diff revision 1)
Show all issues
```
One-line/sentence summary here too.
And below.
```

reviewboard/static/rb/js/ui/views/menuButtonView.ts (Diff revision 1)

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.

reviewboard/static/rb/js/ui/views/menuButtonView.ts (Diff revision 1)

Show all issues

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

david Feb. 3, 2023, 9:57 p.m.

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.

reviewboard/static/rb/js/ui/views/menuButtonView.ts (Diff revision 1)

Show all issues

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

reviewboard/static/rb/js/ui/views/menuButtonView.ts (Diff revision 1)
Show all issues
```
No need to call the parent here. It's explicitly empty.
```

reviewboard/static/rb/js/ui/views/menuButtonView.ts (Diff revision 1)

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.

reviewboard/static/rb/js/ui/views/menuButtonView.ts (Diff revision 1)

Show all issues

Can we sort this?

david Feb. 3, 2023, 9:57 p.m.

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

reviewboard/static/rb/js/ui/views/menuView.es6.js (Diff revision 1)
Show all issues
```
This shouldn't be indented.
```
reviewboard/static/rb/js/ui/views/menuView.es6.js (Diff revision 1)
Show all issues
```
We should also include Version Added here.
```

reviewboard/static/rb/js/views/draftReviewBannerView.es6.js (Diff revision 1)

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.)

reviewboard/static/rb/js/views/draftReviewBannerView.es6.js (Diff revision 1)
Show all issues
```
Leftover debugging.
```
reviewboard/static/rb/js/views/draftReviewBannerView.es6.js (Diff revision 1)
Show all issues
```
Leftover debugging.
```

Commits:

	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.	69595a56dc02ac599b9afa939771c491d48a2542
	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

Diff:

Revision 2 (+514 -580)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/ui/index.ts
	reviewboard/static/rb/js/ui/views/menuButtonView.es6.js
	reviewboard/static/rb/js/ui/views/menuView.es6.js
	reviewboard/static/rb/js/ui/views/splitButtonView.es6.js
	reviewboard/static/rb/js/ui/views/tests/menuButtonViewTests.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/static/rb/js/ui/index.ts (Diff revision 2)
Show all issues
```
Can we put this in alphabetical order?
```

reviewboard/static/rb/js/ui/views/menuView.es6.js (Diff revision 2)

Show all issues

These should be combined, updated for bullet points.

Status: Closed (submitted)

Change Summary:

Pushed to release-6.x (2d165c6)