Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hotkey to open selected notification in new tab #1240

Merged
merged 22 commits into from
Apr 6, 2018

Conversation

Maxscores
Copy link
Contributor

@Maxscores Maxscores commented Apr 2, 2018

Fixes #1110

Adds feature where a user can use 'shift o' on a highlighted notification to open the notification in a new tab.

Maxscores and others added 6 commits March 30, 2018 18:10
Co-Authored-By: Tyler Madsen <madsen.tyler@gmail.com>
Co-Authored-By: Alex Barnes <abarnes26@users.noreply.github.com>
Co-Authored-By: Tyler Madsen <madsen.tyler@gmail.com>
Co-Authored-By: Alex Barnes <abarnes26@users.noreply.github.com>

document.addEventListener('keyup', event => {
const notification = select('.js-notification, .navigation-focus');
const isO = (event.key === 'O');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be inline in the if statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, definitely..

document.addEventListener('keyup', event => {
const notification = select('.js-notification, .navigation-focus');
const isO = (event.key === 'O');
const url = document.querySelector('.navigation-focus').querySelector('a');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

document.querySelector('.navigation-focus').querySelector('a'); => document.querySelector('.navigation-focus a');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better, thanks

import {registerShortcut} from './improve-shortcut-help';

export default function () {
registerShortcut('notifications', 'shift o', 'Open Notification in New Tab');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open Notification in New Tab => Open selected notification in new tab?

Copy link
Contributor Author

@Maxscores Maxscores Apr 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sindresorhus is definitely more clear, thanks for taking the time to review!

@sindresorhus sindresorhus changed the title Open Notification in New Tab Add hotkey to open selected notification in new tab Apr 3, 2018
Co-Authored-By: Tyler Madsen <madsen.tyler@gmail.com>
Co-Authored-By: Alex Barnes <abarnes26@users.noreply.github.com>
@fregante fregante assigned fregante and unassigned fregante Apr 3, 2018
@fregante fregante self-requested a review April 3, 2018 15:21
@fregante
Copy link
Member

fregante commented Apr 3, 2018

GitHub itself seems to have this feature in the code, but I can't seem to enable it. It should work via Ctrl+Enter. Does it work on Windows?

@yakov116
Copy link
Member

yakov116 commented Apr 3, 2018

@bfred-it where should I check? it worked

@Maxscores
Copy link
Contributor Author

@bfred-it 'CTRL + Enter' Doesn't work on my Mac

@fregante
Copy link
Member

fregante commented Apr 3, 2018

@bfred-it it worked

Windows, I imagine?

So this feature is Mac-specific + should be activated via ctrl enter as well, unless anyone else can replicate on Mac somehow too.

@yakov116
Copy link
Member

yakov116 commented Apr 3, 2018

@bfred-it windows

I am not sure it worked. Where should I be doing it on the notification screen?

@fregante
Copy link
Member

fregante commented Apr 3, 2018

Where should I be doing it on the notification screen?

On the notification screen, select a notification via j/k and then press ctrl+enter; the notification should open in a new tab. If you just press enter or o it should just open it in the current tab.

@Maxscores
Copy link
Contributor Author

How would I go about adding a listener for 'ctrl - enter'? Is there a way to listen for key combos? From what I'm seeing is that the keyCode is the same when I press enter whether or not I have ctrl pressed.

@yakov116
Copy link
Member

yakov116 commented Apr 3, 2018

@bfred-it I'm slow lol. I forgot it should open in a new tab. Nope it's not working for me

Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be renamed to be non-notifications-specific. It can work across the site.

window.open(url, '_blank');
document.addEventListener('keypress', event => {
const selected = document.querySelector('.navigation-focus .js-navigation-open[href]');
if (selected && event.key === 'O') {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, that's a much more concise way of doing it

Copy link
Member

@yakov116 yakov116 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you forgot the .js

@fregante
Copy link
Member

fregante commented Apr 3, 2018

Thanks @yakov116!

@Maxscores
Copy link
Contributor Author

@bfred-it Added description and gif of interaction to the readme

@fregante
Copy link
Member

fregante commented Apr 6, 2018

@Maxscores thanks for this PR ^_^

@fregante fregante merged commit 1e24281 into refined-github:master Apr 6, 2018
@vanniktech
Copy link
Contributor

vanniktech commented Apr 24, 2018

Is it possible to open the new tab but stay in the notifications?

My use case is that I go through all of the notifications, select the ones I want to go deeper into with shift + o and then once they've been cleared I'll switch tabs.

@vanniktech
Copy link
Contributor

Another things is that when you press shift + o they're not marked as read immediately. Instead they still appear in the list and only when refreshing they'll be gone.

@fregante
Copy link
Member

Is it possible to open the new stab but stay in the notifications?

Possible, but you probably shouldn't stab anyone before reading their messages.

Yes, I'd prefer the same. Instead of window.open(selected, '_blank'); it can use this https://github.com/sindresorhus/refined-github/blob/c7497caa5f602a8a0ec28564833611761ef00bd7/source/features/open-all-selected.js#L21-L24

Another things is that when you press shift + o they're not marked as read immediately

Can be fixed by replacing the unread class with read.

PR welcome!

@vanniktech
Copy link
Contributor

Can be fixed by replacing the unread class with read.

Should not we just click on the Mark as read button?

@vanniktech
Copy link
Contributor

cc @bfred-it

@fregante
Copy link
Member

Each click triggers an ajax request. We should probably use classList.replace in open-all-notifications

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants