Summary

Convert RB.CommitView to TS/spina and Ink.

Review Request #14105 — Created Aug. 15, 2024 and updated Sept. 6, 2024, 7:53 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change converts the CommitView class to TypeScript, and replaces

various UI elements with Ink. In particular:

Several icons have been switched from fontawesome to Tabler icons

  bundled in Ink.
The progress spinner when creating a post-commit review request now

  uses <Ink.Spinner>.
The confirmation dialog uses <Ink.Dialog>

This also fixes a couple ugly interaction issues relating to what is

happening when during the event handler. The big problem was that we

were always creating the confirmation dialog and then handling other

cases in the confirm handler. This meant that for commits that had an

existing review request, we'd prompt them to create a new review request

and then just open the existing one. For inaccessible commits, we'd

prompt them and then do nothing. These conditions are now checked at the

top of the click handler, so we don't prompt the user unless we really

need to.

Testing Done


Ran js-tests.
Created new review requests for commits and saw that the interaction

  worked correctly for both confirm and cancel.
Clicked on commits with existing review requests and saw that the page

  linked directly without any prompt.
Verified the appearance of the new icons and spinner.

Commits

Summary	ID
Convert RB.CommitView to TS/spina and Ink. This change converts the CommitView class to TypeScript, and replaces various UI elements with Ink. In particular: - Several icons have been switched from fontawesome to Tabler icons bundled in Ink. - The progress spinner when creating a post-commit review request now uses `<Ink.Spinner>`. - The confirmation dialog uses `<Ink.Dialog>` This also fixes a couple ugly interaction issues relating to what is happening when during the event handler. The big problem was that we were always creating the confirmation dialog and then handling other cases in the confirm handler. This meant that for commits that had an existing review request, we'd prompt them to create a new review request and then just open the existing one. For inaccessible commits, we'd prompt them and then do nothing. These conditions are now checked at the top of the click handler, so we don't prompt the user unless we really need to. Testing Done: - Ran js-tests. - Created new review requests for commits and saw that the interaction worked correctly for both confirm and cancel. - Clicked on commits with existing review requests and saw that the page linked directly without any prompt. - Verified the appearance of the new icons and spinner.	292619f6593c29af18fd7eed7a85283afa3fedeb

Summary

Convert RB.CommitView to TS/spina and Ink.

This change converts the CommitView class to TypeScript, and replaces various UI elements with Ink. In particular: - Several icons have been switched from fontawesome to Tabler icons bundled in Ink. - The progress spinner when creating a post-commit review request now uses `<Ink.Spinner>`. - The confirmation dialog uses `<Ink.Dialog>` This also fixes a couple ugly interaction issues relating to what is happening when during the event handler. The big problem was that we were always creating the confirmation dialog and then handling other cases in the confirm handler. This meant that for commits that had an existing review request, we'd prompt them to create a new review request and then just open the existing one. For inaccessible commits, we'd prompt them and then do nothing. These conditions are now checked at the top of the click handler, so we don't prompt the user unless we really need to. Testing Done: - Ran js-tests. - Created new review requests for commits and saw that the interaction worked correctly for both confirm and cancel. - Clicked on commits with existing review requests and saw that the page linked directly without any prompt. - Verified the appearance of the new icons and spinner.

292619f6593c29af18fd7eed7a85283afa3fedeb

Files

Issues

Description	From	Last Updated
These have vertical alignment issues. I think they're across the whole row, but they're very noticeable on the clock. (May …	chipx86	Sept. 3, 2024, 1:50 p.m.
Looks like a 1 snuck in.	chipx86	Sept. 3, 2024, 1:18 p.m.
I remember we had a discussion before about </> vs. the full name. Do we want to revisit what we …	chipx86	Sept. 3, 2024, 1:50 p.m.
There's some low-hanging fruit we can do to improve accessibility here. Maybe best as a new change, but we should …	chipx86	Sept. 6, 2024, 7:52 a.m.
We access this.model 5 times. Let's pull it out into a local variable.	chipx86	Sept. 6, 2024, 7:53 a.m.
We access this.model 5 times. Let's pull it out into a local variable.	chipx86	Sept. 6, 2024, 7:53 a.m.

flake8 passed.

JSHint passed.

Commits list with new icons/spinner (Revision 1)

The issue has been resolved. Show all issues

These have vertical alignment issues. I think they're across the whole row, but they're very noticeable on the clock. (May have been there before, but we should fix them.)

reviewboard/static/rb/js/newReviewRequest/views/commitView.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Looks like a 1 snuck in.
```

reviewboard/static/rb/js/newReviewRequest/views/commitView.ts (Diff revision 1)

The issue has been resolved. Show all issues

I remember we had a discussion before about </> vs. the full name. Do we want to revisit what we want to standardize on here? This craft template is inconsistent in which is used.

david Sept. 3, 2024, 2:20 p.m.

I don't remember that discussion, but I think we should probably always include the full name.

chipx86 Sept. 5, 2024, 2:56 p.m.

It was on Slack when I was first building Ink's craft/paint templates. Worth noting, there is one case where </> is a requirement, and that's when using <{SomeClass} ...> as the opening tag, referencing an explicit class to craft.

Commits:

	Summary	ID
	Convert RB.CommitView to TS/spina and Ink. This change converts the CommitView class to TypeScript, and replaces various UI elements with Ink. In particular: - Several icons have been switched from fontawesome to Tabler icons bundled in Ink. - The progress spinner when creating a post-commit review request now uses `<Ink.Spinner>`. - The confirmation dialog uses `<Ink.Dialog>` This also fixes a couple ugly interaction issues relating to what is happening when during the event handler. The big problem was that we were always creating the confirmation dialog and then handling other cases in the confirm handler. This meant that for commits that had an existing review request, we'd prompt them to create a new review request and then just open the existing one. For inaccessible commits, we'd prompt them and then do nothing. These conditions are now checked at the top of the click handler, so we don't prompt the user unless we really need to. Testing Done: - Ran js-tests. - Created new review requests for commits and saw that the interaction worked correctly for both confirm and cancel. - Clicked on commits with existing review requests and saw that the page linked directly without any prompt. - Verified the appearance of the new icons and spinner.	e544a11faeb0a40ee937638c88847c0c452e49b8
	Convert RB.CommitView to TS/spina and Ink. This change converts the CommitView class to TypeScript, and replaces various UI elements with Ink. In particular: - Several icons have been switched from fontawesome to Tabler icons bundled in Ink. - The progress spinner when creating a post-commit review request now uses `<Ink.Spinner>`. - The confirmation dialog uses `<Ink.Dialog>` This also fixes a couple ugly interaction issues relating to what is happening when during the event handler. The big problem was that we were always creating the confirmation dialog and then handling other cases in the confirm handler. This meant that for commits that had an existing review request, we'd prompt them to create a new review request and then just open the existing one. For inaccessible commits, we'd prompt them and then do nothing. These conditions are now checked at the top of the click handler, so we don't prompt the user unless we really need to. Testing Done: - Ran js-tests. - Created new review requests for commits and saw that the interaction worked correctly for both confirm and cancel. - Clicked on commits with existing review requests and saw that the page linked directly without any prompt. - Verified the appearance of the new icons and spinner.	1a38bf6badcef1ae0c5826ad978f7ff82502f264

Diff:

Revision 2 (+364 -346)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/pages/newReviewRequest.less
	reviewboard/static/rb/js/newReviewRequest/index.ts
	reviewboard/static/rb/js/newReviewRequest/views/commitView.ts
	reviewboard/static/rb/js/newReviewRequest/views/commitView.es6.js
	reviewboard/static/rb/js/newReviewRequest/views/index.ts

Added Files:

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/newReviewRequest/views/commitView.ts (Diff revision 2)

The issue has been dropped. Show all issues

There's some low-hanging fruit we can do to improve accessibility here. Maybe best as a new change, but we should track this (we need an Asana project somewhere for this).

david Sept. 6, 2024, 7:53 a.m.

Added to the a11y section in our backlog.

reviewboard/static/rb/js/newReviewRequest/views/commitView.ts (Diff revision 2)
The issue has been resolved. Show all issues
```
We access this.model 5 times. Let's pull it out into a local variable.
```
reviewboard/static/rb/js/newReviewRequest/views/commitView.ts (Diff revision 2)
The issue has been resolved. Show all issues
```
We access this.model 5 times. Let's pull it out into a local variable.
```

Commits:

	Summary	ID
	Convert RB.CommitView to TS/spina and Ink. This change converts the CommitView class to TypeScript, and replaces various UI elements with Ink. In particular: - Several icons have been switched from fontawesome to Tabler icons bundled in Ink. - The progress spinner when creating a post-commit review request now uses `<Ink.Spinner>`. - The confirmation dialog uses `<Ink.Dialog>` This also fixes a couple ugly interaction issues relating to what is happening when during the event handler. The big problem was that we were always creating the confirmation dialog and then handling other cases in the confirm handler. This meant that for commits that had an existing review request, we'd prompt them to create a new review request and then just open the existing one. For inaccessible commits, we'd prompt them and then do nothing. These conditions are now checked at the top of the click handler, so we don't prompt the user unless we really need to. Testing Done: - Ran js-tests. - Created new review requests for commits and saw that the interaction worked correctly for both confirm and cancel. - Clicked on commits with existing review requests and saw that the page linked directly without any prompt. - Verified the appearance of the new icons and spinner.	1a38bf6badcef1ae0c5826ad978f7ff82502f264
	Convert RB.CommitView to TS/spina and Ink. This change converts the CommitView class to TypeScript, and replaces various UI elements with Ink. In particular: - Several icons have been switched from fontawesome to Tabler icons bundled in Ink. - The progress spinner when creating a post-commit review request now uses `<Ink.Spinner>`. - The confirmation dialog uses `<Ink.Dialog>` This also fixes a couple ugly interaction issues relating to what is happening when during the event handler. The big problem was that we were always creating the confirmation dialog and then handling other cases in the confirm handler. This meant that for commits that had an existing review request, we'd prompt them to create a new review request and then just open the existing one. For inaccessible commits, we'd prompt them and then do nothing. These conditions are now checked at the top of the click handler, so we don't prompt the user unless we really need to. Testing Done: - Ran js-tests. - Created new review requests for commits and saw that the interaction worked correctly for both confirm and cancel. - Clicked on commits with existing review requests and saw that the page linked directly without any prompt. - Verified the appearance of the new icons and spinner.	292619f6593c29af18fd7eed7a85283afa3fedeb

Diff:

Revision 3 (+372 -346)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/pages/newReviewRequest.less
	reviewboard/static/rb/js/newReviewRequest/index.ts
	reviewboard/static/rb/js/newReviewRequest/views/commitView.ts
	reviewboard/static/rb/js/newReviewRequest/views/commitView.es6.js
	reviewboard/static/rb/js/newReviewRequest/views/index.ts

Checks run (2 succeeded)

flake8 passed.

JSHint passed.