Add `keyboard-navigation` feature #3242

yakov116 · 2020-06-17T00:21:53Z

LINKED ISSUES:
Closes Navigating Issues/PRs with keyboard shortcuts #1270
TEST URLS:
Navigating Issues/PRs with keyboard shortcuts #1270
SCREENSHOT:
None

Adds the following shortcuts:

	j: 'Move up a comment',
	k: 'Move down a comment',
	e: 'Edit the focused comment',
	d: 'Delete the focused comment'

source/features/comment-shortcuts.tsx

Not the review discussion comments

source/features/comment-shortcuts.tsx

yakov116 · 2020-06-18T02:41:33Z

I tested it looks like its working. Will work on in tomorrow morning some more.

Thanks for reviewing!

fregante

There's 4-part suggestion to split the shortcutClass.has(event.key) check in 2 pieces (since you're already checking these keys specifically: ['j', 'k'].includes(event.key))

source/features/comment-shortcuts.tsx

Co-Authored-By: Fregante <opensource@bfred.it>

This reverts commit 71d2e1b.

And it works

fregante

Here's a requested change, but reviews can't be minimized.

source/features/comment-shortcuts.tsx

yakov116 · 2020-06-18T20:49:39Z

@fregante I spent about 2 hours trying to figure out another solution.

I need the popup to open, for review comments editing and the hide to work.

Maybe you can help me.

Side note: I think the feature is fully functional and working well

source/features/comment-shortcuts.tsx

This reverts commit d673891.

yakov116 · 2020-06-28T04:37:39Z

source/features/keyboard-navigation.tsx

+		const items = select.all('.js-targetable-element')
+			.filter(comment => !comment.querySelector('.minimized-comment:not(.d-none)'));


This filter does not work correctly it needs to check also for '.js-minimizable-comment-group'.
Can I do the following?

Suggested change

const items = select.all('.js-targetable-element')

.filter(comment => !comment.querySelector('.minimized-comment:not(.d-none)'));

const itemsSelectors = pageDetect.isPRFiles ? '.js-targetable-element' : '.js-minimizable-comment-group'

const items = select.all(itemsSelectors)

.filter(comment => !comment.querySelector('.minimized-comment:not(.d-none)'));

If by "does not work correctly" you mean "it highlights reviews but they don't have a highlight outline", then the solution could be:

.js-targetable-element:not([id^="pullrequestreview"])

That's fine. Minimized comments are still comments. A future shortcut could open them

.js-targetable-element:not([id^="pullrequestreview"])

Let's also use this

fregante

Sorry that this PR is taking so long, but I want to get it right 😅

Honestly I'd also drop the e/d shortcuts for now. I feel that:

file/comment navigation is already super useful on its own
on their own, delete/edit aren't actions one needs to take that frequently.

We could discuss this separately and merge this before tomorrow's release if those shortcuts are temporarily dropped.

The following suggestions can be applied at once to remove those 2 shortcuts

source/features/keyboard-navigation.tsx

fregante · 2020-07-01T15:30:07Z

readme.md

 - [](# "minimize-upload-bar") [Reduces the upload bar to a small button.](https://user-images.githubusercontent.com/55841/59802383-3d994180-92e9-11e9-835d-60de67611c30.png)
 - [](# "clean-rich-text-editor") [Hides unnecessary comment field tooltips and toolbar items](https://user-images.githubusercontent.com/1402241/53629083-a4fe8900-3c47-11e9-8211-bfe2d254ffcb.png) (each one has a keyboard shortcut.)
 - [](# "monospace-textareas") Use a monospace font for all textareas.
+- [](# "keyboard-navigation") Adds shortcuts to comments: <kbd>j</kbd> focuses the comment below; <kbd>k</kbd> focuses the comment above; <kbd>e</kbd> edits the focused comment; <kbd>d</kbd> deletes the focused comment.


Suggested change

- [](# "keyboard-navigation") Adds shortcuts to comments: <kbd>j</kbd> focuses the comment below; <kbd>k</kbd> focuses the comment above; <kbd>e</kbd> edits the focused comment; <kbd>d</kbd> deletes the focused comment.

- [](# "keyboard-navigation") Adds shortcuts to comments and PR files: <kbd>j</kbd> focuses the comment below..

readme.md

Co-authored-by: Fregante <opensource@bfred.it>

yakov116 · 2020-07-01T18:24:42Z

Sorry that this PR is taking so long, but I want to get it right 😅

Thanks for working with me!!

Applied will test later tonight

yakov116 · 2020-07-02T00:23:39Z

Tested. Its good with me except for #3242 (comment)

source/features/keyboard-navigation.tsx

fregante

I haven't re-tested it, but it looks good

yakov116 · 2020-07-03T13:56:13Z

FTR this is what I am talking about. That it randomly opens minimized reviews when using only the .js-targetable-element selector.

But lets deal with this issue another time.

source/features/keyboard-navigation.tsx

Co-authored-by: Fregante <opensource@bfred.it>

readme.md

source/features/keyboard-navigation.tsx

fregante · 2020-07-06T08:42:43Z

You made it! I opened a new issue for the remaining concerns: #3319

yakov116 added 5 commits June 16, 2020 17:53

Add comment-shortcuts

cc179ba

Work when selecting a comment

9fdce13

Add comment

0166489

Add jk

d24ebd2

Attempt to support review comment shortcuts

5bf6df4

notlmn suggested changes Jun 17, 2020

View reviewed changes

source/features/comment-shortcuts.tsx Outdated Show resolved Hide resolved

source/features/comment-shortcuts.tsx Outdated Show resolved Hide resolved

j k for reviewComments (The approval and changes requested)

27647e9

Not the review discussion comments

This comment has been minimized.

Sign in to view

fregante requested changes Jun 18, 2020

View reviewed changes

Start feedback

41f119b

yakov116 requested a review from fregante June 18, 2020 02:40

fregante requested changes Jun 18, 2020

View reviewed changes

fregante added the enhancement label Jun 18, 2020

yakov116 and others added 3 commits June 18, 2020 07:58

Apply Review Suggestions

c1acf68

Co-Authored-By: Fregante <opensource@bfred.it>

Filter out minimized comments

71d2e1b

Revert "Filter out minimized comments "

80133c5

This reverts commit 71d2e1b.

This comment has been minimized.

Sign in to view

yakov116 added 2 commits June 18, 2020 08:41

Merge remote-tracking branch 'upstream/master' into comment-shortcuts

c8500a9

Filter out minimized comments

d169fda

And it works

fregante requested changes Jun 18, 2020

View reviewed changes

source/features/comment-shortcuts.tsx Outdated Show resolved Hide resolved

Make hide and edit on review comments

d673891

yakov116 requested a review from fregante June 18, 2020 20:44

fregante reviewed Jun 18, 2020

View reviewed changes

source/features/comment-shortcuts.tsx Outdated Show resolved Hide resolved

fregante reviewed Jun 18, 2020

View reviewed changes

source/features/comment-shortcuts.tsx Outdated Show resolved Hide resolved

Revert "Make hide and edit on review comments"

90a80ab

This reverts commit d673891.

fregante mentioned this pull request Jun 22, 2020

Fix GitHub's buggy "scroll to focused comment" feature #3263

Closed

yakov116 added 3 commits June 26, 2020 08:11

Merge remote-tracking branch 'upstream/master' into comment-shortcuts

90b5d4a

Drop scrollToView

6bfce39

Use js-targetable-element

bfe60d4

yakov116 commented Jun 28, 2020

View reviewed changes

fregante approved these changes Jul 1, 2020

View reviewed changes

Apply suggestions from code review

c9992f2

Co-authored-by: Fregante <opensource@bfred.it>

Merge remote-tracking branch 'upstream/master' into comment-shortcuts

4be1d69

notlmn reviewed Jul 2, 2020

View reviewed changes

source/features/keyboard-navigation.tsx Show resolved Hide resolved

notlmn reviewed Jul 2, 2020

View reviewed changes

source/features/keyboard-navigation.tsx Outdated Show resolved Hide resolved

fregante reviewed Jul 2, 2020

View reviewed changes

yakov116 added 2 commits July 3, 2020 09:41

Merge branch 'master' into comment-shortcuts

1c132f3

Stay put on same index

aa38357

fregante reviewed Jul 5, 2020

View reviewed changes

source/features/keyboard-navigation.tsx Outdated Show resolved Hide resolved

Update source/features/keyboard-navigation.tsx

cb1697a

Co-authored-by: Fregante <opensource@bfred.it>

fregante reviewed Jul 5, 2020

View reviewed changes

readme.md Outdated Show resolved Hide resolved

yakov116 and others added 2 commits July 5, 2020 17:05

Exclude pull request reviews

7770108

Merge branch 'master' into comment-shortcuts

787967f

fregante reviewed Jul 6, 2020

View reviewed changes

source/features/keyboard-navigation.tsx Outdated Show resolved Hide resolved

Add screenshot

a07ea79

fregante merged commit b5c240b into refined-github:master Jul 6, 2020

fregante mentioned this pull request Jul 6, 2020

keyboard-navigation issues #3319

Closed

yakov116 deleted the comment-shortcuts branch July 6, 2020 11:50

dertieran mentioned this pull request Jul 21, 2020

Fix keyboard-navigation issues #3377

Merged

fregante mentioned this pull request Sep 8, 2020

Button to move to next change in diff #3537

Closed

yakov116 mentioned this pull request Nov 13, 2020

Add a keyboard shortcut open the dropdown on the focused comments #3719

Closed

		const items = select.all('.js-targetable-element')
		.filter(comment => !comment.querySelector('.minimized-comment:not(.d-none)'));

	- [](# "keyboard-navigation") Adds shortcuts to comments: <kbd>j</kbd> focuses the comment below; <kbd>k</kbd> focuses the comment above; <kbd>e</kbd> edits the focused comment; <kbd>d</kbd> deletes the focused comment.
	- [](# "keyboard-navigation") Adds shortcuts to comments and PR files: <kbd>j</kbd> focuses the comment below..

Uh oh!

Add keyboard-navigation feature #3242

Add keyboard-navigation feature #3242

Uh oh!

Conversation

yakov116 commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yakov116 commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fregante left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

fregante left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yakov116 commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yakov116 Jun 28, 2020

Choose a reason for hiding this comment

Uh oh!

fregante Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

This comment was marked as abuse.

Uh oh!

fregante Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fregante Jul 5, 2020

Choose a reason for hiding this comment

Uh oh!

fregante left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fregante Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yakov116 commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yakov116 commented Jul 2, 2020

Uh oh!

Uh oh!

Uh oh!

Add `keyboard-navigation` feature #3242

Add `keyboard-navigation` feature #3242

yakov116 commented Jun 17, 2020 •

edited

Loading

yakov116 commented Jun 18, 2020 •

edited

Loading

fregante left a comment •

edited

Loading

yakov116 commented Jun 18, 2020 •

edited

Loading

fregante Jul 2, 2020 •

edited

Loading

fregante left a comment •

edited

Loading

yakov116 commented Jul 1, 2020 •

edited

Loading