[WIP] Add ability to collapse entire files in the diff view.
Review Request #9337 — Created Oct. 27, 2017 and discarded
Information | |
---|---|
rcreagha | |
Review Board | |
release-3.0.x | |
Reviewers | |
reviewboard, students | |
When viewing the diff of a review request not all files need to be
viewed, for example minified files don't need to be reviewed. It is
also helpful to have a way to hide files you have already reviewed.Now files can be collapsed and expanded as the user wishes. Also,
minified files are collapsed by default.In a previous commit clicking the new button whould hide the diff
information as well as the links to download the file. I have changed
it no longer hide the download links so that it matches files hidden
with the "Hide/Show whitespace changes" button.I modified the function toggleWhitespaceOnlyChunks in
diffReviewableView.es6.js so it now takes in a boolean value to indicate
if the files should be shown or hidden. I made this change because I needed
the "Hide/Show whitespace changes" button to work with the button to hide/show
a specific file, and without specifying if files should be shown or hidden it
would have caused issues where clicking "Hide whitespace changes" would hide
some but show others if they were already individually hidden with the new
button.
Unit tests have not been written yet.
Also it might be worth mentioning that because the existing
"Collapse changes" button reloads the page, any files that were
manually collapsed or expanded will return to their default state.
Description | From | Last Updated |
---|---|---|
If there's a newly-added minified filename, I think depot_filename will be None, which might crash. This should also use single … |
|
|
This comment should have an "Args" section explaining what e is. |
|
|
Because this is ES6, we can use a slightly shorter syntax: _toggleCollapseDiff(e) { |
|
|
These should use const instead of var. |
|
|
Should be indented only 1 space within its parent. |
|
|
E127 continuation line over-indented for visual indent |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
This is really difficult to read. Since collapse_by_default is a boolean value, I think it makes more sense to assign … |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
Looking at the other if tags in the file, they all have a space before closing the tag: ...show_deleted %} |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 22 Expected '===' and instead saw '=='. |
![]() |
|
Col: 46 Missing semicolon. |
![]() |
-
-
reviewboard/diffviewer/diffutils.py (Diff revision 1) If there's a newly-added minified filename, I think
depot_filename
will beNone
, which might crash. This should also use single quotes, and probably test for.min.
. -
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 1) This comment should have an "Args" section explaining what
e
is. -
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 1) Because this is ES6, we can use a slightly shorter syntax:
_toggleCollapseDiff(e) {
-
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 1) These should use
const
instead ofvar
. -
reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 1) Should be indented only 1 space within its parent.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+71 -11) |
Checks run (2 succeeded)
-
-
Don't know if this is helpful at all, or even a part of the requirements of your project, but if you did want to maintain the collapsed state of your elements even after page reload, you could save a list of collapsed file ids under an object in browser storage (probably local storage) indexed by the current review request id. If the list under the review request id exists, use it to determine which elements should be collapsed on client-side loading. If it doesn't, remove all other currently saved review request collapsed-lists (to not use up a bunch of local storage space).
Change Summary:
Revamped the collapsing logic to work better with cases like deleted files and whitespace changes
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+104 -20) |
Checks run (1 failed, 1 succeeded)
flake8
-
reviewboard/diffviewer/renderers.py (Diff revision 3) E127 continuation line over-indented for visual indent
-
reviewboard/diffviewer/renderers.py (Diff revision 3) E127 continuation line over-indented for visual indent
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
Change Summary:
Added the new button to the diffs for files that have been moved or copied
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+111 -21) |
Checks run (1 failed, 1 succeeded)
flake8
-
reviewboard/diffviewer/renderers.py (Diff revision 4) E127 continuation line over-indented for visual indent
-
reviewboard/diffviewer/renderers.py (Diff revision 4) E127 continuation line over-indented for visual indent

-
Just a couple code formatting things I noticed.
-
reviewboard/diffviewer/renderers.py (Diff revision 4) This is really difficult to read. Since
collapse_by_default
is a boolean value, I think it makes more sense to assign it as a boolean instead of changing it's value in anif
statement.I'm thinking something like this:
collapse_by_default = (self.diff_file['minified'] and not self.diff_file['binary'] and not self.diff_file['deleted'] and (not self.diff_file['moved_or_copied'] or self.diff_file['num_changes'] != 0) and (not self.diff_file['new_file'] or self.diff_file['num_chunks'] != 0))
-
reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 4) Looking at the other if tags in the file, they all have a space before closing the tag:
...show_deleted %}
Change Summary:
Fixed bugs created earier in this project
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+126 -24) |
Checks run (2 failed)
flake8
-
reviewboard/diffviewer/renderers.py (Diff revision 5) E128 continuation line under-indented for visual indent
-
reviewboard/diffviewer/renderers.py (Diff revision 5) E128 continuation line under-indented for visual indent
-
reviewboard/diffviewer/renderers.py (Diff revision 5) E128 continuation line under-indented for visual indent
-
reviewboard/diffviewer/renderers.py (Diff revision 5) E128 continuation line under-indented for visual indent
-
reviewboard/diffviewer/renderers.py (Diff revision 5) E126 continuation line over-indented for hanging indent
JSHint
-
reviewboard/static/rb/js/diffviewer/views/chunkHighlighterView.js (Diff revision 5) Col: 22 Expected '===' and instead saw '=='.
-
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 5) Col: 46 Missing semicolon.