Add the ability to toggle display of entire files.
Review Request #11210 — Created Oct. 5, 2020 and updated
Information | |
---|---|
keanweng | |
Review Board | |
release-4.0.x | |
Reviewers | |
reviewboard, students | |
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 | |
---|---|
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) |