Summary

Archive review requests from inbox icon

Review Request #10154 — Created Sept. 21, 2018 and submitted Jan. 10, 2019, 11:50 a.m.

Information

Owner

Malcolm

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

9afd7cf...

Reviewers

Groups

reviewboard, students

People

Description

Made the inbox icon on review request pages a shortcut for
archiving/unarchiving the review request.

The idea behind this change is to make the archiving/unarchiving of
requests a little bit faster for advanced users.

The shortcut was implemented by adding archive/unarchive click handlers
to the icon. A title attribute was also added to the icon which
dynamically changes to "Archive review request" or "Unarchive review
request". This should help with accessibility and general ease of use.

Testing Done

There were no changes made to any Python files so the Python tests were
not ran for this change.

The JS tests were ran for these changes.
These changes were implemented off the master branch version of
ReviewBoard which had 6 failures for the JS tests.

The JS tests when ran on this update had the same 6 failures as in the
master branch JS tests.

Issues

Description	From	Last Updated
Can you add some title= text to the inbox icon that says "Archive" ?	brennie	Sept. 23, 2018, 7:31 a.m.
Please update the description to include a little bit more information about the motivation for this change.	david	Sept. 23, 2018, 7:55 a.m.
Please fill out the "Testing done" field explaining what you have done to test the change.	david	Sept. 23, 2018, 7:55 a.m.
Can you limit your summary to 50 characters or so?	brennie	Sept. 29, 2018, 3:59 p.m.
Can you rewrite your summary to be in the imperitive mood, i.e., as if it were a command or order? …	brennie	Sept. 29, 2018, 4:59 p.m.
Can you wrap your description & testing done at 72 characters?	brennie	Sept. 29, 2018, 5:09 p.m.
This is an es6 file, so we can (and should) have a comma on the last item in the object.	david	Sept. 23, 2018, 7:31 a.m.
Your code has trailing whitespace. You'll want to remove this.	brennie	Sept. 29, 2018, 6:27 p.m.
This should be let over `var.	brennie	Sept. 29, 2018, 6:27 p.m.
Instead of switching on the iconClass, I would do what the calculation for iconClass does, e.g.: const iconTitle = (visibility …	brennie	Sept. 29, 2018, 6:13 p.m.
These need to use gettext('...') so that the strings are marked for translation.	brennie	Sept. 29, 2018, 6:13 p.m.
Since these are classes, they aren't necessarily unique. It is possible that the rb-icon-archive-{on,off} class could be used for another …	brennie	Oct. 25, 2018, 7:48 p.m.
We're now doing the same equality check three times. How about adding: const visible = (visibility === RB.ReviewRequest.VISIBILITY_VISIBLE);	david	Jan. 10, 2019, 11:50 a.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Can you add some title= text to the inbox icon that says "Archive" ?

The issue has been resolved. Show all issues

Please update the description to include a little bit more information about the motivation for this change.

The issue has been resolved. Show all issues

Please fill out the "Testing done" field explaining what you have done to test the change.

reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
This is an es6 file, so we can (and should) have a comma on the last item in the object.
```

```
nn
```

Description:

		Made the inbox icon on review request pages a shortcut for archiving/unarchiving the review request.
	+
	+	The idea behind this change is to make the archiving/unarchiving of requests a little bit faster for advanced users.
	+
	+	The shortcut was implemented by adding archive/unarchive click handlers to the icon. A title attribute was also added to the icon which dynamically changes to "Archive review request" or "Unarchive review request". This should help with accessibility and general ease of use.

Testing Done:

	+	There were no changes made to any Python files so the Python tests were not ran for this change.
	+
	+	The JS tests were ran for these changes.
	+	These changes were implemented off the master branch version of ReviewBoard which had 6 failures for the JS tests.
	+
	+	The JS tests when ran on this update had the same 6 failures as in the master branch JS tests.

Commit:

-	570c6e94279146a0bd3eafe17458b5f1c039790b
+	11e336e03010bc7906b5df311459b812e131cf32

Diff:

Revision 2 (+13 -1)

Show changes

reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

-	11e336e03010bc7906b5df311459b812e131cf32
+	e7dbc7c5c51f2794e7b2e64877e121d95500637b

Diff:

Revision 3 (+13 -1)

Show changes

reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Can you limit your summary to 50 characters or so?

The issue has been resolved. Show all issues

Can you rewrite your summary to be in the imperitive mood, i.e., as if it were a command or order?

If you substitute your summary into the following sentence, it should make sense:

This patch will <summary>

The issue has been resolved. Show all issues

Can you wrap your description & testing done at 72 characters?

reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 2)

The issue has been resolved. Show all issues

Your code has trailing whitespace. You'll want to remove this.

reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
This should be let over `var.
```

reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 2)

The issue has been resolved. Show all issues

Instead of switching on the iconClass, I would do what the calculation for iconClass does, e.g.:


const iconTitle = (visibility === RB.ReviewRequest.VISIBILITY_VISIBLE
                   ? gettext('Archive review request')
                   : gettext('Unarchive review request'));


This is because the underlying case is a boolean, which are easier to compare compared to strings (which at worst compare N bytes of an N byte string instead of 1).
Also, at first glance, there is no default case so iconTitle could end up undefined.

reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
These need to use gettext('...') so that the strings are marked for translation.
```

Summary:

-	Made the inbox icon on review request pages a shortcut for archiving/unarchiving the review request.
+	Archive review requests from inbox icon

Description:

~		Made the inbox icon on review request pages a shortcut for archiving/unarchiving the review request.
	~	Made the inbox icon on review request pages a shortcut for
	+	archiving/unarchiving the review request.

~		The idea behind this change is to make the archiving/unarchiving of requests a little bit faster for advanced users.
	~	The idea behind this change is to make the archiving/unarchiving of
	+	requests a little bit faster for advanced users.

~		The shortcut was implemented by adding archive/unarchive click handlers to the icon. A title attribute was also added to the icon which dynamically changes to "Archive review request" or "Unarchive review request". This should help with accessibility and general ease of use.
	~	The shortcut was implemented by adding archive/unarchive click handlers
	+	to the icon. A title attribute was also added to the icon which
	+	dynamically changes to "Archive review request" or "Unarchive review
	+	request". This should help with accessibility and general ease of use.

Testing Done:

~		There were no changes made to any Python files so the Python tests were not ran for this change.
	~	There were no changes made to any Python files so the Python tests were
	+	not ran for this change.

		The JS tests were ran for these changes.
~		These changes were implemented off the master branch version of ReviewBoard which had 6 failures for the JS tests.
	~	These changes were implemented off the master branch version of
	+	ReviewBoard which had 6 failures for the JS tests.

~		The JS tests when ran on this update had the same 6 failures as in the master branch JS tests.
	~	The JS tests when ran on this update had the same 6 failures as in the
	+	master branch JS tests.

Commit:

-	e7dbc7c5c51f2794e7b2e64877e121d95500637b
+	20684076665693995aa323f41ae5f41388a0d214

Diff:

Revision 4 (+7 -1)

Show changes

reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 4)

The issue has been resolved. Show all issues

Since these are classes, they aren't necessarily unique. It is possible that the rb-icon-archive-{on,off} class could be used for another icon somewhere in the editor view.
So let's make them unique by adding an ID to this.

Change Summary:

Added id to the archiving shortcut.

Commit:

-	20684076665693995aa323f41ae5f41388a0d214
+	9afd7cf3dbd51e0c4db757ae13d35b29eadb781e

Diff:

Revision 5 (+10 -1)

Show changes

reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 5)

The issue has been resolved. Show all issues

We're now doing the same equality check three times. How about adding:

const visible = (visibility === RB.ReviewRequest.VISIBILITY_VISIBLE);

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (dbd469f)