david got a fish trophy!
Convert reviewable views to TypeScript/spina.
Review Request #13531 — Created Feb. 12, 2024 and submitted
This change converts the reviewable views (and associated utility views)
to TypeScript/spina.While I was here I disconvered that one of the tests for DiffReviewable
was intended to be async, but didn't take adone
callback and was
therefore skipping the meat of the test. After fixing that, it became
evident that the implementation of the test was broken, so it's been
rewritten.
- Ran js-tests.
- Smoke tested reviewing diffs and file attachments.
Summary | ID |
---|---|
b532967c60148688cb442555fba4439cebbcc1c3 |
Description | From | Last Updated |
---|---|---|
Nit: I notice there's still some places where we use RB.DiffReviewableView in docstrings (mostly for argument types and return types), … |
maubin | |
These are missing docs. |
maubin | |
Do we want any of this to be in onInitialRender? |
chipx86 | |
Can we document these? |
chipx86 | |
Can we document this? |
chipx86 | |
We're using this style elsewhere: export class CenteredElementManager extends BaseView< ..., ... > { ... } |
chipx86 | |
Should be Partial<...>. |
chipx86 | |
Should we move this into onInitialRender? |
chipx86 | |
Can we list the version this will be removed? |
chipx86 | |
Missing docs. |
chipx86 | |
The new docs need a Version Added (for anything new, of course). |
chipx86 | |
Can we document these, since they're meant to be filled in by subclasses? |
chipx86 | |
Can you add docs for this? |
chipx86 | |
Hmm, I should have exposed this in Spina, if we need this. Do we, though? We're only going to use … |
chipx86 | |
Should be Partial<...>. |
chipx86 |
-
-
reviewboard/static/rb/js/reviews/views/textBasedReviewableView.ts (Diff revision 1) These are missing docs.
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+3446 -2372) |
Checks run (2 succeeded)
-
This looks great. I just spotted a few missing docs, some possible
render()
->onInitialRender()
cases, and a couple cases where we should usePartial<...>
for options. -
reviewboard/static/rb/js/reviews/views/diffReviewableView.ts (Diff revision 2) Do we want any of this to be in
onInitialRender
? -
reviewboard/static/rb/js/ui/views/centeredElementManager.ts (Diff revision 2) Can we document these?
-
-
reviewboard/static/rb/js/ui/views/centeredElementManager.ts (Diff revision 2) We're using this style elsewhere:
export class CenteredElementManager extends BaseView< ..., ... > { ... }
-
reviewboard/static/rb/js/ui/views/centeredElementManager.ts (Diff revision 2) Should be
Partial<...>
. -
reviewboard/static/rb/js/reviews/views/abstractReviewableView.ts (Diff revision 2) Should we move this into
onInitialRender
? -
reviewboard/static/rb/js/reviews/views/abstractReviewableView.ts (Diff revision 2) Can we list the version this will be removed?
-
-
reviewboard/static/rb/js/reviews/views/imageReviewableView.ts (Diff revision 2) Can we document these, since they're meant to be filled in by subclasses?
-
reviewboard/static/rb/js/reviews/views/imageReviewableView.ts (Diff revision 2) Can you add docs for this?
-
reviewboard/static/rb/js/reviews/views/textCommentRowSelectorView.ts (Diff revision 2) Should be
Partial<...>
.
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+3514 -2396) |
Checks run (2 succeeded)
-
-
reviewboard/static/rb/js/reviews/views/imageReviewableView.ts (Diff revisions 2 - 3) The new docs need a
Version Added
(for anything new, of course). -
reviewboard/static/rb/js/reviews/views/textCommentRowSelectorView.ts (Diff revisions 2 - 3) Hmm, I should have exposed this in Spina, if we need this.
Do we, though? We're only going to use what's in our own options. The default Backbone ones are going to be for things like
tagName
andclassName
, and presumably we don't care about this withininitialize
(it won't affect construction, which always takes the merged versions).
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+3556 -2430) |