[WIP] Keyboard shortcuts for expand & collapse diff
Review Request #10951 — Created March 12, 2020 and updated
Information | |
---|---|
hxqlin | |
Review Board | |
master | |
10925 | |
a4627d4... | |
Reviewers | |
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 thee
key and collapse is triggered by thew
key (thec
key may seem more
natural, but it's taken already by another shortcut sow
was chosen for its
proximity to thee
key for ease of use between them).For the diff viewer, expand and collapse buttons are now tracked as anchors
inRB.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. Thee
andw
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, thee
andw
keys will trigger their respective expand and collapse buttons. This
is implemented by using the key bindings mixin inRB.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
andw
key events in both the diff
fragment and diff viewer.
Description | From | Last Updated |
---|---|---|
W291 trailing whitespace |
![]() |
|
Can you update this to describe what this means? "Next" from what viewpoint? Same below. |
|
|
Arguments should align. |
|
|
Arguments should align. |
|
|
Blank line between a statement and a block. |
|
|
Function doc summaries must be a single unwrapped line. Always good to expand on meanings in subsequent paragraphs. |
|
|
Querying based on attributes is slow, and that's very important here, because such a query could end up taking an … |
|
|
This can be: label = `'${key}'`; |
|
Change Summary:
Implement keyboard shortcuts in diff fragments
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+87 -18) |
Checks run (1 failed, 1 succeeded)
flake8
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 2) Show all issues
Change Summary:
Add unit tests
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+133 -18) |
Checks run (2 succeeded)
-
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 3) Can you update this to describe what this means? "Next" from what viewpoint?
Same below.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 3) Arguments should align.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 3) Arguments should align.
-
reviewboard/static/rb/js/views/diffFragmentView.es6.js (Diff revision 3) Blank line between a statement and a block.
-
reviewboard/static/rb/js/views/diffFragmentView.es6.js (Diff revision 3) Function doc summaries must be a single unwrapped line.
Always good to expand on meanings in subsequent paragraphs.
-
reviewboard/static/rb/js/views/diffFragmentView.es6.js (Diff revision 3) 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.)
-
reviewboard/static/rb/js/views/tests/diffFragmentViewTests.es6.js (Diff revision 3) This can be:
label = `'${key}'`;
-
-
reviewboard/static/rb/js/views/diffFragmentView.es6.js (Diff revision 3) 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. -
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. -
reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 3) part of the dependent change
-
reviewboard/templates/reviews/diff_comment_fragment.html (Diff revision 3) part of the dependent change
-
reviewboard/templates/reviews/diff_comment_fragment.html (Diff revision 3) part of the dependent change
-
reviewboard/templates/reviews/diff_comment_fragment.html (Diff revision 3) 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.
Change Summary:
Fix issues from review and update description.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+135 -19) |