Summary

Add the ability to toggle display of entire files.

Review Request #11210 — Created Oct. 5, 2020 and updated Dec. 4, 2020, 1:58 a.m.

Information

Owner

keanweng

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

The current diff viewer shows the changes for all files and it can be 

tedious to scroll through, especially with large changes. Adding the 

ability to hide/unhide files will better allow users to look at only 

a subset of the changes at once.
We added a toggle to the header for each file that collapses the file 

to a single line filename. Toggling a collapsed file header will expand 

the entire file. This feature is referenced from the toggling of 

Reviews in a Review Request. When the toggle button is clicked, 

jQuery toggle() is called on the revision-row class and tbody 

attribute. The toggle() function works toggling the CSS display 

property between display: none.

Testing Done

Wrote onToggleCollapseClicked in diffReviewableViewTests.es6.js.

Commits

Summary	ID
The current diff viewer shows the changes for all files and it can be tedious to scroll through, especially with large changes. Adding the ability to hide/unhide files will better allow users to look at only a subset of the changes at once. We added a toggle to the header for each file that collapses the file to a single line filename. Toggling a collapsed file header will expand the entire file. This feature is referenced from the toggling of `Reviews` in a `Review Request`. When the toggle button is clicked, jQuery `toggle()` is called on the `revision-row` class and `tbody` attribute. The `toggle()` function works toggling the CSS `display` property between `display: none`. Testing done: Wrote `onToggleCollapseClicked` in `diffReviewableViewTests.es6.js`.	579ed9843edaa7523cf2d7b60fdd8a2f66ac3bbb

Summary

The current diff viewer shows the changes for all files and it can be

tedious to scroll through, especially with large changes. Adding the ability to hide/unhide files will better allow users to look at only a subset of the changes at once. We added a toggle to the header for each file that collapses the file to a single line filename. Toggling a collapsed file header will expand the entire file. This feature is referenced from the toggling of `Reviews` in a `Review Request`. When the toggle button is clicked, jQuery `toggle()` is called on the `revision-row` class and `tbody` attribute. The `toggle()` function works toggling the CSS `display` property between `display: none`. Testing done: Wrote `onToggleCollapseClicked` in `diffReviewableViewTests.es6.js`.

579ed9843edaa7523cf2d7b60fdd8a2f66ac3bbb

Files

Issues

Description	From	Last Updated
Hey Kean, can you please add some screenshots of what these toggles look like?	mike_conley	Oct. 18, 2020, 5:42 p.m.
In order to start moving towards our new class naming standards, can we call the state class on the elements …	david	Oct. 23, 2020, 6:15 p.m.
A couple small comments on the look here: Where this same combination is used for reviews, the button is square …	david	Oct. 23, 2020, 1:14 p.m.
I'm confused why this is necessary. Aren't you setting the correct icon class in the HTML template?	david	Oct. 23, 2020, 1:14 p.m.
This blank line can be removed.	david	Oct. 23, 2020, 1:16 p.m.
This can be simplified a bit: const collapsed = this._$filenameRow.hasClass('collapsed'); this._$expandCollpaseButton .toggleClass('rb-icon-expand-review', collapsed) .toggleClass('rb-icon-collapse-review', !collapsed)	david	Oct. 23, 2020, 1:16 p.m.
This blank line can be removed.	david	Oct. 23, 2020, 1:15 p.m.
Let's add a trailing comma to this line.	david	Oct. 23, 2020, 1:15 p.m.
I know it was like this previously, but can you remove the spaces inside {{ file.depot_filename }} so it's consistent …	david	Oct. 23, 2020, 1:15 p.m.
Col: 66 Missing semicolon.	reviewbot	Oct. 22, 2020, 5:55 p.m.
Col: 64 Missing semicolon.	reviewbot	Oct. 22, 2020, 5:55 p.m.
Backbone views have an attribute called this.$el, so you can just use that instead of re-wrapping in jquery.	david	Oct. 23, 2020, 6:26 p.m.
Pretty close. Class name should be -is-collapsed (note the leading hyphen).	david	Oct. 23, 2020, 6:18 p.m.
This blank line can go away.	david	Oct. 23, 2020, 6:16 p.m.

flake8 passed.

JSHint passed.

Commits:

	Summary	ID
	Add toggle button and function for diff files.	fbeec8382b711654a5822e852463d2ded66be71e
	Add toggle button and function for diff files.	fbeec8382b711654a5822e852463d2ded66be71e
	Add JS test for expand toggle diff files.	763b8a0adde47974a200732bbd1de610abe449c8

Diff:

Revision 2 (+106 -4)

Show changes

	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.es6.js
	reviewboard/templates/diffviewer/diff_file_fragment.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:: ~
Add toggle button and function for diff files.
~
The current diff viewer shows the changes for all files and it can be
+ tedious to scroll through, especially with large changes. Adding the
+ ability to hide/unhide files will better allow users to look at only
+ a subset of the changes at once.
+
+
We added a toggle to the header for each file that collapses the file
+ to a single line filename. Toggling a collapsed file header will expand
+ the entire file. This feature is referenced from the toggling of
+ Reviews in a Review Request. When the toggle button is clicked,
+ jQuery toggle() is called on the revision-row class and tbody
+ attribute. The toggle() function works toggling the CSS display
+ property between display: none.
Testing Done:: +
Wrote onToggleCollapseClicked in diffReviewableViewTests.es6.js.

Summary:

[WIP] Add the ability to toggle display of entire files.

Add the ability to toggle display of entire files.

Commits:

	Summary	ID
	Add toggle button and function for diff files.	fbeec8382b711654a5822e852463d2ded66be71e
	Add JS test for expand toggle diff files.	763b8a0adde47974a200732bbd1de610abe449c8
	The current diff viewer shows the changes for all files and it can be tedious to scroll through, especially with large changes. Adding the ability to hide/unhide files will better allow users to look at only a subset of the changes at once. We added a toggle to the header for each file that collapses the file to a single line filename. Toggling a collapsed file header will expand the entire file. This feature is referenced from the toggling of `Reviews` in a `Review Request`. When the toggle button is clicked, jQuery `toggle()` is called on the `revision-row` class and `tbody` attribute. The `toggle()` function works toggling the CSS `display` property between `display: none`. Testing done: Wrote `onToggleCollapseClicked` in `diffReviewableViewTests.es6.js`.	6f5e66f4461b400bd082cf8fab5592673398e5fd

Diff:

Revision 3 (+106 -4)

Show changes

	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.es6.js
	reviewboard/templates/diffviewer/diff_file_fragment.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Initialize view for onToggleCollapseClicked JS test

Commits:

	Summary	ID
	The current diff viewer shows the changes for all files and it can be tedious to scroll through, especially with large changes. Adding the ability to hide/unhide files will better allow users to look at only a subset of the changes at once. We added a toggle to the header for each file that collapses the file to a single line filename. Toggling a collapsed file header will expand the entire file. This feature is referenced from the toggling of `Reviews` in a `Review Request`. When the toggle button is clicked, jQuery `toggle()` is called on the `revision-row` class and `tbody` attribute. The `toggle()` function works toggling the CSS `display` property between `display: none`. Testing done: Wrote `onToggleCollapseClicked` in `diffReviewableViewTests.es6.js`.	6f5e66f4461b400bd082cf8fab5592673398e5fd
	The current diff viewer shows the changes for all files and it can be tedious to scroll through, especially with large changes. Adding the ability to hide/unhide files will better allow users to look at only a subset of the changes at once. We added a toggle to the header for each file that collapses the file to a single line filename. Toggling a collapsed file header will expand the entire file. This feature is referenced from the toggling of `Reviews` in a `Review Request`. When the toggle button is clicked, jQuery `toggle()` is called on the `revision-row` class and `tbody` attribute. The `toggle()` function works toggling the CSS `display` property between `display: none`. Testing done: Wrote `onToggleCollapseClicked` in `diffReviewableViewTests.es6.js`.	6ae899ab5c45852c6c2fb3bc577a5a0aeb765ec0

Diff:

Revision 4 (+130 -4)

Show changes

	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.es6.js
	reviewboard/templates/diffviewer/diff_file_fragment.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Hey Kean, can you please add some screenshots of what these toggles look like?

keanweng Oct. 17, 2020, 4:39 p.m.

Added screenshots of the toggles, the icons are based on Review Request's toggle.

Added Files:

The issue has been resolved. Show all issues

In order to start moving towards our new class naming standards, can we call the state class on the elements -is-collapsed instead of collapsed?

not collapsed.png (Revision 1)

The issue has been resolved. Show all issues

A couple small comments on the look here:

Where this same combination is used for reviews, the button is square around the icon. Something is stretching it out a bit here, and I think we should dig into the styles to figure out why and fix it.
Can we add a little bit of spacing between the button and the filename? The review entry CSS adds margin-right: 0.5rem on the .collapse-btn element.

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

The issue has been resolved. Show all issues

I'm confused why this is necessary. Aren't you setting the correct icon class in the HTML template?

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 4)
The issue has been resolved. Show all issues
```
This blank line can be removed.
```

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

The issue has been resolved. Show all issues

This can be simplified a bit:

const collapsed = this._$filenameRow.hasClass('collapsed');
this._$expandCollpaseButton
    .toggleClass('rb-icon-expand-review', collapsed)
    .toggleClass('rb-icon-collapse-review', !collapsed)

reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.es6.js (Diff revision 4)
The issue has been resolved. Show all issues
```
This blank line can be removed.
```
reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.es6.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Let's add a trailing comma to this line.
```

reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 4)

The issue has been resolved. Show all issues

I know it was like this previously, but can you remove the spaces inside {{ file.depot_filename }} so it's consistent with the rest of the templates?

Change Summary:

Fix diff4 issues highlighted by David.

Commits:

	Summary	ID
	The current diff viewer shows the changes for all files and it can be tedious to scroll through, especially with large changes. Adding the ability to hide/unhide files will better allow users to look at only a subset of the changes at once. We added a toggle to the header for each file that collapses the file to a single line filename. Toggling a collapsed file header will expand the entire file. This feature is referenced from the toggling of `Reviews` in a `Review Request`. When the toggle button is clicked, jQuery `toggle()` is called on the `revision-row` class and `tbody` attribute. The `toggle()` function works toggling the CSS `display` property between `display: none`. Testing done: Wrote `onToggleCollapseClicked` in `diffReviewableViewTests.es6.js`.	6ae899ab5c45852c6c2fb3bc577a5a0aeb765ec0
	The current diff viewer shows the changes for all files and it can be tedious to scroll through, especially with large changes. Adding the ability to hide/unhide files will better allow users to look at only a subset of the changes at once. We added a toggle to the header for each file that collapses the file to a single line filename. Toggling a collapsed file header will expand the entire file. This feature is referenced from the toggling of `Reviews` in a `Review Request`. When the toggle button is clicked, jQuery `toggle()` is called on the `revision-row` class and `tbody` attribute. The `toggle()` function works toggling the CSS `display` property between `display: none`. Testing done: Wrote `onToggleCollapseClicked` in `diffReviewableViewTests.es6.js`.	ba72e3e4aa1a7eb71bde37151715329d632a48d7

Diff:

Revision 5 (+122 -4)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.es6.js
	reviewboard/templates/diffviewer/diff_file_fragment.html

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 66
 Missing semicolon.
```
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 64
 Missing semicolon.
```

Change Summary:

Fix JSHint issue.

Commits:

	Summary	ID
	The current diff viewer shows the changes for all files and it can be tedious to scroll through, especially with large changes. Adding the ability to hide/unhide files will better allow users to look at only a subset of the changes at once. We added a toggle to the header for each file that collapses the file to a single line filename. Toggling a collapsed file header will expand the entire file. This feature is referenced from the toggling of `Reviews` in a `Review Request`. When the toggle button is clicked, jQuery `toggle()` is called on the `revision-row` class and `tbody` attribute. The `toggle()` function works toggling the CSS `display` property between `display: none`. Testing done: Wrote `onToggleCollapseClicked` in `diffReviewableViewTests.es6.js`.	ba72e3e4aa1a7eb71bde37151715329d632a48d7
	The current diff viewer shows the changes for all files and it can be tedious to scroll through, especially with large changes. Adding the ability to hide/unhide files will better allow users to look at only a subset of the changes at once. We added a toggle to the header for each file that collapses the file to a single line filename. Toggling a collapsed file header will expand the entire file. This feature is referenced from the toggling of `Reviews` in a `Review Request`. When the toggle button is clicked, jQuery `toggle()` is called on the `revision-row` class and `tbody` attribute. The `toggle()` function works toggling the CSS `display` property between `display: none`. Testing done: Wrote `onToggleCollapseClicked` in `diffReviewableViewTests.es6.js`.	105b2102251d80a4caaff036f39d5692af188bae

Diff:

Revision 6 (+122 -4)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.es6.js
	reviewboard/templates/diffviewer/diff_file_fragment.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Removed Files:
Added Files:

You can go ahead and mark as fixed out any issues from previous reviews that have already been addressed.

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Backbone views have an attribute called this.$el, so you can just use that instead of re-wrapping in jquery.
```
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Pretty close. Class name should be -is-collapsed (note the leading hyphen).
```
reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.es6.js (Diff revision 6)
The issue has been resolved. Show all issues
```
This blank line can go away.
```

Commits:

	Summary	ID
	The current diff viewer shows the changes for all files and it can be tedious to scroll through, especially with large changes. Adding the ability to hide/unhide files will better allow users to look at only a subset of the changes at once. We added a toggle to the header for each file that collapses the file to a single line filename. Toggling a collapsed file header will expand the entire file. This feature is referenced from the toggling of `Reviews` in a `Review Request`. When the toggle button is clicked, jQuery `toggle()` is called on the `revision-row` class and `tbody` attribute. The `toggle()` function works toggling the CSS `display` property between `display: none`. Testing done: Wrote `onToggleCollapseClicked` in `diffReviewableViewTests.es6.js`.	105b2102251d80a4caaff036f39d5692af188bae
	The current diff viewer shows the changes for all files and it can be tedious to scroll through, especially with large changes. Adding the ability to hide/unhide files will better allow users to look at only a subset of the changes at once. We added a toggle to the header for each file that collapses the file to a single line filename. Toggling a collapsed file header will expand the entire file. This feature is referenced from the toggling of `Reviews` in a `Review Request`. When the toggle button is clicked, jQuery `toggle()` is called on the `revision-row` class and `tbody` attribute. The `toggle()` function works toggling the CSS `display` property between `display: none`. Testing done: Wrote `onToggleCollapseClicked` in `diffReviewableViewTests.es6.js`.	579ed9843edaa7523cf2d7b60fdd8a2f66ac3bbb

Diff:

Revision 7 (+120 -4)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.es6.js
	reviewboard/templates/diffviewer/diff_file_fragment.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

I pulled down the patch and everything looks good and agrees with the screenshots. Good job!