Add the ability to toggle display of entire files.
Review Request #11210 — Created Oct. 5, 2020 and updated
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 aReview Request
. When the toggle button is clicked,
jQuerytoggle()
is called on therevision-row
class andtbody
attribute. Thetoggle()
function works toggling the CSSdisplay
property betweendisplay: none
.
Wrote
onToggleCollapseClicked
indiffReviewableViewTests.es6.js
.
Summary | ID |
---|---|
579ed9843edaa7523cf2d7b60fdd8a2f66ac3bbb |
Description | From | Last Updated |
---|---|---|
Hey Kean, can you please add some screenshots of what these toggles look like? |
|
|
In order to start moving towards our new class naming standards, can we call the state class on the elements … |
|
|
A couple small comments on the look here: Where this same combination is used for reviews, the button is square … |
|
|
I'm confused why this is necessary. Aren't you setting the correct icon class in the HTML template? |
|
|
This blank line can be removed. |
|
|
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) |
|
|
This blank line can be removed. |
|
|
Let's add a trailing comma to this line. |
|
|
I know it was like this previously, but can you remove the spaces inside {{ file.depot_filename }} so it's consistent … |
|
|
Col: 66 Missing semicolon. |
![]() |
|
Col: 64 Missing semicolon. |
![]() |
|
Backbone views have an attribute called this.$el, so you can just use that instead of re-wrapping in jquery. |
|
|
Pretty close. Class name should be -is-collapsed (note the leading hyphen). |
|
|
This blank line can go away. |
|
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+106 -4) |
Checks run (2 succeeded)
Description: |
|
|||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
Summary: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
|||||||||
Diff: |
Revision 3 (+106 -4) |
Checks run (2 succeeded)
Change Summary:
Initialize view for
onToggleCollapseClicked
JS test
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+130 -4) |
Checks run (2 succeeded)
-
-
In order to start moving towards our new class naming standards, can we call the state class on the elements
-is-collapsed
instead ofcollapsed
? -
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) 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) This blank line can be removed.
-
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 4) 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/templates/diffviewer/diff_file_fragment.html (Diff revision 4) 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: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+122 -4) |
Checks run (1 failed, 1 succeeded)
JSHint
-
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 5) Col: 66 Missing semicolon.
-
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 5) Col: 64 Missing semicolon.
Change Summary:
Fix JSHint issue.
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+122 -4) |
Checks run (2 succeeded)
-
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) 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) Pretty close. Class name should be
-is-collapsed
(note the leading hyphen). -
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+120 -4) |