Port RB.MenuButtonView to TypeScript and spina, remove SplitButtonView.
Review Request #12824 — Created Feb. 3, 2023 and submitted
Information | |
---|---|
david | |
Review Board | |
release-6.x | |
Reviewers | |
reviewboard | |
This change ports the MenuButtonView UI element over to our new
TypeScript/spina framework.The
MenuButtonView
had mostly replaced the oldSplitButtonView
,
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 | |
---|---|
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 … |
|
|
One-line/sentence summary here too. And below. |
|
|
Blank line between these. Makes it easier to document the instance variables when public. And actually, we should probably stick … |
|
|
Wonder if we should start making these static. I can't remember if there are currently any issues using that. |
|
|
I don't think we need to call the parent when subclassing BaseView/View. |
|
|
No need to call the parent here. It's explicitly empty. |
|
|
We should use ... as Element instead of <Element>.... This is the recommended syntax in TypeScript going forward (designed to … |
|
|
Can we sort this? |
|
|
This shouldn't be indented. |
|
|
We should also include Version Added here. |
|
|
We can use the _ template literal stuff for these gettext calls. (_ makes ESLint grumble a bit, but I … |
|
|
Leftover debugging. |
|
|
Leftover debugging. |
|
|
Can we put this in alphabetical order? |
|
|
These should be combined, updated for bullet points. |
|
-
-
reviewboard/static/rb/js/ui/views/menuButtonView.ts (Diff revision 1) 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) One-line/sentence summary here too.
And below.
-
reviewboard/static/rb/js/ui/views/menuButtonView.ts (Diff revision 1) 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) Wonder if we should start making these static. I can't remember if there are currently any issues using that.
-
reviewboard/static/rb/js/ui/views/menuButtonView.ts (Diff revision 1) 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) No need to call the parent here. It's explicitly empty.
-
reviewboard/static/rb/js/ui/views/menuButtonView.ts (Diff revision 1) 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/menuView.es6.js (Diff revision 1) We should also include
Version Added
here. -
reviewboard/static/rb/js/views/draftReviewBannerView.es6.js (Diff revision 1) We can use the
_
template literal stuff for thesegettext
calls.(
_
makes ESLint grumble a bit, but I plan to address that.) -
-
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+514 -580) |
Checks run (2 succeeded)
-
-
-
reviewboard/static/rb/js/ui/views/menuView.es6.js (Diff revision 2) These should be combined, updated for bullet points.