[WIP] Keyboard shortcuts for expand & collapse diff

Review Request #10951 — Created March 12, 2020 and updated

hxqlin
Review Board
master
10925
a4627d4...
reviewboard

This patch adds keyboard shortcuts to expand & collapse the diff in a review
request and in a diff fragment in a review. Expand is triggered by the e
key and collapse is triggered by the w key (the c key may seem more
natural, but it's taken already by another shortcut so w was chosen for its
proximity to the e key for ease of use between them).

For the diff viewer, expand and collapse buttons are now tracked as anchors
in RB.DiffViewerPageView. This allows us to find the next button on the
page from the currently selected anchor and trigger it when a keyboard
shortcut is used. The e and w keys are added to the existing key
bindings mixin object to track those key presses and trigger the right
button.

For diff fragments, there can only be one set of expand buttons and one
collapse button in any given fragment. If the fragment is focused, the e
and w keys will trigger their respective expand and collapse buttons. This
is implemented by using the key bindings mixin in RB.DiffFragmentView. The
shortcut to expand the fragment defaults to expanding the diff by 20 lines,
which is achieved by adding a modifier to desired expand button.

Manual testing & unit tests for the e and w key events in both the diff
fragment and diff viewer.

Description From Last Updated

W291 trailing whitespace

reviewbotreviewbot

Can you update this to describe what this means? "Next" from what viewpoint? Same below.

chipx86chipx86

Arguments should align.

chipx86chipx86

Arguments should align.

chipx86chipx86

Blank line between a statement and a block.

chipx86chipx86

Function doc summaries must be a single unwrapped line. Always good to expand on meanings in subsequent paragraphs.

chipx86chipx86

Querying based on attributes is slow, and that's very important here, because such a query could end up taking an ...

chipx86chipx86

This can be: label = `'${key}'`;

chipx86chipx86
hxqlin
Review request changed
hxqlin
chipx86
  1. 
      
  2. Can you update this to describe what this means? "Next" from what viewpoint?

    Same below.

  3. Arguments should align.

  4. Arguments should align.

  5. Blank line between a statement and a block.

  6. Function doc summaries must be a single unwrapped line.

    Always good to expand on meanings in subsequent paragraphs.

  7. Querying based on attributes is slow, and that's very important here, because such a query could end up taking an actual noticeable amount of time for larger diffs. It'd be better to specifically look for and cache any such thing up-front, if we can, or to at least narrow down first by something more specific.

    Classes are also faster to query.

    Also, how is this finding the "next" one? This looks like it'd find all of them and click all of them. (Same with others.)

    1. https://reviews.reviewboard.org/r/10951/#comment50645 - updated the selector to use a class instead of an attribute, but the reasoning is the same

    2. Sorry this is a bit hard to follow - this is the primary comment I meant to reply to. I'll re-explain everything here. So after all the updated changes, here's why this would only click one button. You said that there will only be one collapse button or set of expand buttons in a diff fragment, so if we look for buttons within a diff fragment, we know we'll have selected a button corresponding to the current fragment. We also only trigger the shortcut if a particular diff fragment is focused so the buttons won't be triggered in the wrong fragment.

      For expand buttons, there will be four buttons in a diff fragment in total but we only want to trigger one of them for the shortcut. I added a new modifier class -is-default to the expand button that I chose to be the default for this shortcut, which is the one that will expand the diff by 20 lines. I chose this one since I thought it would be the most common use case out of the expand buttons, but feel free to correct me. So when we look for an expand button with that modifier within a diff fragment, we'll only find one.

      For the collapse button, there will only be one of them in a diff fragment if there's one at all. Again, since we only trigger the shortcut for the current fragment if it's focused and we search for the button within the focused fragment, it will only find the collapse button for the current fragment if it exists and trigger it or do nothing at all.

      Hope that made more sense!

  8. This can be:

    label = `'${key}'`;
    
  9. 
      
hxqlin
  1. 
      
    1. I think you accidentally posted a new review, instead of replying to mine, so I'm not sure exactly what everything pertains to. When you comment in the diff viewer, you're creating a new review, not replying.

  2. I added a comment at the bit of code that adds this [data-default-expand-btn="true"] attribute to the button explaining how this button will be the only one. The reasoning behind how these will only affect one button is because you said there will only be one collapse button or set of expand buttons in a diff fragment, so finding and clicking one within the element will only affect the current fragment.

  3. reviewboard/static/rb/js/views/tests/diffFragmentViewTests.es6.js (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    I forgot to leave some comments regarding this function - it's actually copied over from diffViewerPageViewTests.es6.js. Would it be better to pull this out into a util file somewhere? I wasn't sure if that was the right move or where to put it.

    1. It's fine to duplicate it right now. It would be ideal for us to have some common function someplace, but that'd be a more standalone task, and it isn't worth spending time on right now.

  4. reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 3)
     
     
     
     
     
     

    part of the dependent change

  5. part of the dependent change

  6. part of the dependent change

  7. This is the only expand button that will have the data-default-expand-btn="true" in a particular diff fragment, so it will be the only button that's affected by the keyboard shortcut.

  8. 
      
hxqlin
Review request changed

Change Summary:

Fix issues from review and update description.

Description:

   

This patch adds keyboard shortcuts to expand & collapse the diff in a review

~   request. Expand is triggered by the e key and collapse is triggered by the w
~   key (the c key may seem more natural, but it's taken already by another shortcut
~   so w was chosen for its proximity to the e key for ease of use between them).

  ~ request and in a diff fragment in a review. Expand is triggered by the e
  ~ key and collapse is triggered by the w key (the c key may seem more
  ~ natural, but it's taken already by another shortcut so w was chosen for its
  + proximity to the e key for ease of use between them).

   
~  

This change is built on top of the changes in https://reviews.reviewboard.org/r/10925/,

~   which make the collapse diff button tabbable.

  ~

For the diff viewer, expand and collapse buttons are now tracked as anchors

  ~ in RB.DiffViewerPageView. This allows us to find the next button on the
  + page from the currently selected anchor and trigger it when a keyboard
  + shortcut is used. The e and w keys are added to the existing key
  + bindings mixin object to track those key presses and trigger the right
  + button.

  +
  +

For diff fragments, there can only be one set of expand buttons and one

  + collapse button in any given fragment. If the fragment is focused, the e
  + and w keys will trigger their respective expand and collapse buttons. This
  + is implemented by using the key bindings mixin in RB.DiffFragmentView. The
  + shortcut to expand the fragment defaults to expanding the diff by 20 lines,
  + which is achieved by adding a modifier to desired expand button.

Testing Done:

~  

Manual testing

  ~

Manual testing & unit tests for the e and w key events in both the diff

  + fragment and diff viewer.

Commit:

-b3b0589377ddddd40fc0571df4543d70be6f96d8
+a4627d4e91fcfef95358e9af7b7da926c82b7680

Diff:

Revision 4 (+135 -19)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...