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
	The current diff viewer shows the changes for all files and it can be

Screenshots

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
-	Add toggle button and function for diff files.
+	Add toggle button and function for diff files.
+	Add JS test for expand toggle diff files.

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
-		Add toggle button and function for diff files.
-		Add JS test for expand toggle diff files.
+		The current diff viewer shows the changes for all files and it can be

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
-		The current diff viewer shows the changes for all files and it can be
+		The current diff viewer shows the changes for all files and it can be

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.

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:

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)

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)

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)
Show all issues
```
This blank line can be removed.
```

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

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)
Show all issues
```
This blank line can be removed.
```
reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.es6.js (Diff revision 4)
Show all issues
```
Let's add a trailing comma to this line.
```

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

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
-		The current diff viewer shows the changes for all files and it can be
+		The current diff viewer shows the changes for all files and it can be

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)
Show all issues
```
Col: 66
 Missing semicolon.
```
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 5)
Show all issues
```
Col: 64
 Missing semicolon.
```

Change Summary:

Fix JSHint issue.

Commits:

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

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)

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)
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)
Show all issues
```
This blank line can go away.
```

Commits:

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

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!

Add the ability to toggle display of entire files.

Commits:

Diff:

Checks run (2 succeeded)

Description:

Testing Done:

Summary:

Commits:

Diff:

Checks run (2 succeeded)

Change Summary:

Commits:

Diff:

Checks run (2 succeeded)

Added Files:

Change Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Change Summary:

Commits:

Diff:

Checks run (2 succeeded)

Removed Files:

Added Files:

Commits:

Diff:

Checks run (2 succeeded)