david got a fish trophy!

Summary

Convert reviewable views to TypeScript/spina.

Review Request #13531 — Created Feb. 12, 2024 and submitted Feb. 27, 2024, 8:24 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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 a done 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.

Testing Done


Ran js-tests.
Smoke tested reviewing diffs and file attachments.

Commits

Summary	ID
Convert reviewable views to TypeScript/spina. 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 a `done` 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. Testing Done: - Ran js-tests. - Smoke tested reviewing diffs and file attachments. Reviewed at https://reviews.reviewboard.org/r/13531/	b532967c60148688cb442555fba4439cebbcc1c3

Summary

Convert reviewable views to TypeScript/spina.

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 a `done` 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. Testing Done: - Ran js-tests. - Smoke tested reviewing diffs and file attachments. Reviewed at https://reviews.reviewboard.org/r/13531/

b532967c60148688cb442555fba4439cebbcc1c3

Issues

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	Feb. 15, 2024, 9:42 a.m.
These are missing docs.	maubin	Feb. 15, 2024, 9:07 a.m.
Do we want any of this to be in onInitialRender?	chipx86	Feb. 15, 2024, 9:47 p.m.
Can we document these?	chipx86	Feb. 15, 2024, 9:51 p.m.
Can we document this?	chipx86	Feb. 15, 2024, 9:49 p.m.
We're using this style elsewhere: export class CenteredElementManager extends BaseView< ..., ... > { ... }	chipx86	Feb. 15, 2024, 9:51 p.m.
Should be Partial<...>.	chipx86	Feb. 15, 2024, 10 p.m.
Should we move this into onInitialRender?	chipx86	Feb. 15, 2024, 10 p.m.
Can we list the version this will be removed?	chipx86	Feb. 15, 2024, 10:01 p.m.
Missing docs.	chipx86	Feb. 15, 2024, 10:01 p.m.
The new docs need a Version Added (for anything new, of course).	chipx86	Feb. 19, 2024, 6:35 p.m.
Can we document these, since they're meant to be filled in by subclasses?	chipx86	Feb. 15, 2024, 10:02 p.m.
Can you add docs for this?	chipx86	Feb. 15, 2024, 10:03 p.m.
Hmm, I should have exposed this in Spina, if we need this. Do we, though? We're only going to use …	chipx86	Feb. 19, 2024, 6:35 p.m.
Should be Partial<...>.	chipx86	Feb. 15, 2024, 10:04 p.m.

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/reviews/views/textBasedReviewableView.ts (Diff revision 1)

The issue has been resolved. Show all issues

These are missing docs.

Commits:

	Summary	ID
	Convert reviewable views to TypeScript/spina. 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 a `done` 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. Testing Done: - Ran js-tests. - Smoke tested reviewing diffs and file attachments.	1ff8fae327d60a5ec9a37fd78777901a16d53d2d
	Convert reviewable views to TypeScript/spina. 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 a `done` 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. Testing Done: - Ran js-tests. - Smoke tested reviewing diffs and file attachments.	49e790ad0f6a12bf7c3092912ea1b2d2d1b2ee66

Diff:

Revision 2 (+3446 -2372)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/reviews/index.ts
	reviewboard/static/rb/js/views/abstractReviewableView.es6.js
	reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js
	reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts
	reviewboard/static/rb/js/views/dummyReviewableView.es6.js
	reviewboard/static/rb/js/reviews/views/fileAttachmentReviewableView.ts
	12 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been dropped. Show all issues

Nit: I notice there's still some places where we use RB.DiffReviewableView in docstrings (mostly for argument types and return types), should we switch it all over to DiffReviewableView?

david Feb. 15, 2024, 9:42 a.m.

I think for doc comments we want to keep it as RB., because that's what it ends up getting exposed as in the final bundle.

Ship it!

```
Ship It!
```

This looks great. I just spotted a few missing docs, some possible render() -> onInitialRender() cases, and a couple cases where we should use Partial<...> for options.

reviewboard/static/rb/js/reviews/views/diffReviewableView.ts (Diff revision 2)

The issue has been resolved. Show all issues

Do we want any of this to be in onInitialRender?

reviewboard/static/rb/js/ui/views/centeredElementManager.ts (Diff revision 2)

The issue has been resolved. Show all issues

Can we document these?

reviewboard/static/rb/js/ui/views/centeredElementManager.ts (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we document this?
```

reviewboard/static/rb/js/ui/views/centeredElementManager.ts (Diff revision 2)

The issue has been resolved. Show all issues

We're using this style elsewhere:

export class CenteredElementManager extends BaseView<
    ...,
    ...
> {
    ...
}

reviewboard/static/rb/js/ui/views/centeredElementManager.ts (Diff revision 2)

The issue has been resolved. Show all issues

Should be Partial<...>.

david Feb. 15, 2024, 10:13 p.m.

I actually don't think Partial is correct here. I think the right thing is to have the options interface define any optional fields as optional, and then this should actually be Backbone.CombinedViewConstructorOptions<...>

reviewboard/static/rb/js/reviews/views/abstractReviewableView.ts (Diff revision 2)
The issue has been resolved. Show all issues
```
Should we move this into onInitialRender?
```

reviewboard/static/rb/js/reviews/views/abstractReviewableView.ts (Diff revision 2)

The issue has been resolved. Show all issues

Can we list the version this will be removed?

reviewboard/static/rb/js/reviews/views/imageReviewableView.ts (Diff revision 2)

The issue has been resolved. Show all issues

Missing docs.

reviewboard/static/rb/js/reviews/views/imageReviewableView.ts (Diff revision 2)
The issue has been resolved. Show all issues
```
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)

The issue has been resolved. Show all issues

Can you add docs for this?

reviewboard/static/rb/js/reviews/views/textCommentRowSelectorView.ts (Diff revision 2)
The issue has been dropped. Show all issues
```
Should be Partial<...>.
```
1. david Feb. 15, 2024, 10:13 p.m.
  See above.

Commits:

	Summary	ID
	Convert reviewable views to TypeScript/spina. 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 a `done` 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. Testing Done: - Ran js-tests. - Smoke tested reviewing diffs and file attachments.	49e790ad0f6a12bf7c3092912ea1b2d2d1b2ee66
	Convert reviewable views to TypeScript/spina. 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 a `done` 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. Testing Done: - Ran js-tests. - Smoke tested reviewing diffs and file attachments. Reviewed at https://reviews.reviewboard.org/r/13531/	cd970afea3e126fd29d56391310d41c07118f763

Diff:

Revision 3 (+3514 -2396)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/reviews/index.ts
	reviewboard/static/rb/js/views/abstractReviewableView.es6.js
	reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js
	reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts
	reviewboard/static/rb/js/views/dummyReviewableView.es6.js
	reviewboard/static/rb/js/reviews/views/fileAttachmentReviewableView.ts
	12 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/reviews/views/imageReviewableView.ts (Diff revisions 2 - 3)
The issue has been resolved. Show all issues
```
The new docs need a Version Added (for anything new, of course).
```

reviewboard/static/rb/js/reviews/views/textCommentRowSelectorView.ts (Diff revisions 2 - 3)

The issue has been resolved. Show all issues

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 and className, and presumably we don't care about this within initialize (it won't affect construction, which always takes the merged versions).

Commits:

	Summary	ID
	Convert reviewable views to TypeScript/spina. 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 a `done` 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. Testing Done: - Ran js-tests. - Smoke tested reviewing diffs and file attachments. Reviewed at https://reviews.reviewboard.org/r/13531/	cd970afea3e126fd29d56391310d41c07118f763
	Convert reviewable views to TypeScript/spina. 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 a `done` 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. Testing Done: - Ran js-tests. - Smoke tested reviewing diffs and file attachments. Reviewed at https://reviews.reviewboard.org/r/13531/	b532967c60148688cb442555fba4439cebbcc1c3

Diff:

Revision 4 (+3556 -2430)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/reviews/index.ts
	reviewboard/static/rb/js/views/abstractReviewableView.es6.js
	reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js
	reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts
	reviewboard/static/rb/js/views/dummyReviewableView.es6.js
	reviewboard/static/rb/js/reviews/views/fileAttachmentReviewableView.ts
	12 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.x (5ebe971)