Summary

Port PageView to spina.

Review Request #12937 — Created April 5, 2023 and submitted April 20, 2023, 9:04 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-6.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change ports the base PageView class to spina and TypeScript. As

part of this, the code has been moved into the common subdirectory to

match the bundle that it's a part of.
The one change that was required outside of this class was to update the

DiffViewerPageModel to use preinitialize instead of constructor.

The use of the reserved name constructor has bitten us in the past,

and wasn't working anymore now that the base class was an ES6 class.

Testing Done


Ran js-tests.
Verified that page views still rendered correctly.

Commits

Summary	ID
Port PageView to spina. This change ports the base PageView class to spina and TypeScript. As part of this, the code has been moved into the `common` subdirectory to match the bundle that it's a part of. The one change that was required outside of this class was to update the `DiffViewerPageModel` to use `preinitialize` instead of `constructor`. The use of the reserved name `constructor` has bitten us in the past, and wasn't working anymore now that the base class was an ES6 class. Testing Done: - Ran js-tests. - Verified that page views still rendered correctly.	bf63aeed7cb7febfec71b026b87c46c6acc793c9

Summary

Port PageView to spina.

This change ports the base PageView class to spina and TypeScript. As part of this, the code has been moved into the `common` subdirectory to match the bundle that it's a part of. The one change that was required outside of this class was to update the `DiffViewerPageModel` to use `preinitialize` instead of `constructor`. The use of the reserved name `constructor` has bitten us in the past, and wasn't working anymore now that the base class was an ES6 class. Testing Done: - Ran js-tests. - Verified that page views still rendered correctly.

bf63aeed7cb7febfec71b026b87c46c6acc793c9

Issues

Description	From	Last Updated
Since this was accessible on the prototype, but isn't really an instance variable, I wonder if we should make this …	chipx86	April 18, 2023, 4:39 p.m.
Can we work to document all instance variables going forward?	chipx86	April 18, 2023, 10:43 a.m.
We can return super.remove() instead.	chipx86	April 18, 2023, 10:43 a.m.
Since this has multiple types involved, let's move actionId to its own line, so the :'s don't all run together.	chipx86	April 18, 2023, 10:44 a.m.
Are you able to just import 'jasmine-core' and get the same results? I've resolved linting locally with just an import …	chipx86	April 18, 2023, 10:45 a.m.

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/common/views/pageView.ts (Diff revision 1)

Show all issues

Since this was accessible on the prototype, but isn't really an instance variable, I wonder if we should make this static and include it in @spina({prototypeAttrs: [...]})?

david April 18, 2023, 10:45 a.m.

Will that work with overriding in inherited classes?

david April 18, 2023, 11:11 a.m.

OK, just tested and it does work as long as the subclass also uses @spina({prototypeAttrs: [...]})

chipx86 April 19, 2023, 12:08 a.m.

Oh, that's supposed to inherit. Hmm... That'd be a bug in Spina if it doesn't.

reviewboard/static/rb/js/common/views/pageView.ts (Diff revision 1)

Show all issues

Can we work to document all instance variables going forward?

reviewboard/static/rb/js/common/views/pageView.ts (Diff revision 1)
Show all issues
```
We can return super.remove() instead.
```

reviewboard/static/rb/js/common/views/pageView.ts (Diff revision 1)

Show all issues

Since this has multiple types involved, let's move actionId to its own line, so the :'s don't all run together.

reviewboard/static/rb/js/common/views/tests/pageViewTests.ts (Diff revision 1)

Show all issues

Are you able to just import 'jasmine-core' and get the same results? I've resolved linting locally with just an import 'jasmine', and wasn't sure if it'd work the same way.

david April 18, 2023, 10:45 a.m.
```
No, that doesn't seem to work.
```

Commits:

	Summary	ID
	Port PageView to spina. This change ports the base PageView class to spina and TypeScript. As part of this, the code has been moved into the `common` subdirectory to match the bundle that it's a part of. The one change that was required outside of this class was to update the `DiffViewerPageModel` to use `preinitialize` instead of `constructor`. The use of the reserved name `constructor` has bitten us in the past, and wasn't working anymore now that the base class was an ES6 class. Testing Done: - Ran js-tests. - Verified that page views still rendered correctly.	4b61d130832830223195d265e9b0e02c161cbd6d
	Port PageView to spina. This change ports the base PageView class to spina and TypeScript. As part of this, the code has been moved into the `common` subdirectory to match the bundle that it's a part of. The one change that was required outside of this class was to update the `DiffViewerPageModel` to use `preinitialize` instead of `constructor`. The use of the reserved name `constructor` has bitten us in the past, and wasn't working anymore now that the base class was an ES6 class. Testing Done: - Ran js-tests. - Verified that page views still rendered correctly.	bf63aeed7cb7febfec71b026b87c46c6acc793c9

Diff:

Revision 2 (+354 -190)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/common/index.ts
	reviewboard/static/rb/js/common/models/pageModel.ts
	reviewboard/static/rb/js/pages/views/basePageView.es6.js
	reviewboard/static/rb/js/common/views/tests/index.ts
	reviewboard/static/rb/js/pages/views/tests/basePageViewTests.es6.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.es6.js
	reviewboard/static/rb/js/pages/models/pageModel.es6.js
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-6.x (27a193d)