Add the ability to toggle display of entire files.

Review Request #11210 — Created Oct. 5, 2020 and updated

Information

Review Board
release-4.0.x

Reviewers

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.

Wrote onToggleCollapseClicked in diffReviewableViewTests.es6.js.

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

Description From Last Updated

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

mike_conleymike_conley

In order to start moving towards our new class naming standards, can we call the state class on the elements …

daviddavid

A couple small comments on the look here: Where this same combination is used for reviews, the button is square …

daviddavid

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

daviddavid

This blank line can be removed.

daviddavid

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)

daviddavid

This blank line can be removed.

daviddavid

Let's add a trailing comma to this line.

daviddavid

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

daviddavid

Col: 66 Missing semicolon.

reviewbotreviewbot

Col: 64 Missing semicolon.

reviewbotreviewbot

Backbone views have an attribute called this.$el, so you can just use that instead of re-wrapping in jquery.

daviddavid

Pretty close. Class name should be -is-collapsed (note the leading hyphen).

daviddavid

This blank line can go away.

daviddavid
keanweng
keanweng
keanweng
keanweng
mike_conley
  1. 
      
  2. Show all issues

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

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

  3. 
      
keanweng
david
  1. 
      
  2. 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?

  3. 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.
  4. Show all issues

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

  5. Show all issues

    This blank line can be removed.

  6. 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)
    
  7. Show all issues

    This blank line can be removed.

  8. Show all issues

    Let's add a trailing comma to this line.

  9. 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?

  10. 
      
keanweng
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

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

  2. Show all issues

    Backbone views have an attribute called this.$el, so you can just use that instead of re-wrapping in jquery.

  3. Show all issues

    Pretty close. Class name should be -is-collapsed (note the leading hyphen).

  4. Show all issues

    This blank line can go away.

  5. 
      
keanweng
Review request changed
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

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
bnie
  1. I pulled down the patch and everything looks good and agrees with the screenshots. Good job!

  2.