Add the ability to toggle display of entire files.

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

keanweng
Review Board
release-4.0.x
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 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
The current diff viewer shows the changes for all files and it can be
Loading file attachments...

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. 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. 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. 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. I'm confused why this is necessary. Aren't you setting the correct icon class in the HTML template?

  5. This blank line can be removed.

  6. 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)
    
  7. Let's add a trailing comma to this line.

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

  9. 
      
keanweng
Review request changed

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

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. Backbone views have an attribute called this.$el, so you can just use that instead of re-wrapping in jquery.

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

  4. 
      
keanweng
Review request changed

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...